dev/drupal#200: Performance improvement

[jaapjansma]

Performance improvement

Overview

With this change loading the contact summary on my local install will improve from 700ms to 500ms.

On a production site this had a similar impact where performance went down from 7 seconds to 4 seconds.

Before

The internal:/ path makes the router component in Drupal lookup the routing and check whether it is a valid route. Which means looking up a civicrm route from CiviCRM.

After

The routing component does not lookup whether the route exists.

Comments

See https://lab.civicrm.org/dev/drupal/-/issues/200

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[civibot]

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/drupal/-/issues/200

[demeritcowboy]

This might be ok, but one difference is that it doesn't resolve the same way, for example, drupal node aliases. But the question would be is anyone using CRM_Utils_System::url or similar to construct links like that. What I mean is for example if I have a node with an alias defined in drupal, e.g. mynode1 is the alias for /node/1, then before the patch:

input: node/1 => output: /mynode1

after the patch:

input: node/1 => output: /node/1

This might just be a minor difference for something like aliases. Off the top of my head I'm not able to think of an example with bigger consequences, or why it would be civi code trying to make that url instead of code living in a drupal module.

@jackrabbithanna @MegaphoneJon

[jaapjansma]

@demeritcowboy you are right but if you want to construct a url for a drupal node one would use the Drupal functionality for that and not CRM_Utils_System::url . So my assumption is that it would be safe to replace this. The change is in a large production site already for 4+ months and nothing seems broken.

[demeritcowboy]

Sorry I need to remove the merge-ready because the reason this is fast is because it completely bypasses the CivicrmPathProcessor, which was the focus in civicrm/civicrm-drupal-8#88 and civicrm/civicrm-drupal-8#98. So we need to understand more why that path processor is there.

It's possible it serves no purpose, but that doesn't feel right.

[MegaphoneJon]

CivicrmPathProcessor is important for paths that aren't in civicrm_menu. This includes CiviReport instances and several import URLs, along with a handful of others. See CRM_Core_Menu::get() starting after the call to civicrm_menu.

[demeritcowboy]

Actually I see there's two things - I was only looking at the generation of the link. When you actually visit the link, it still hits the pathprocessor so those links still work (without the processor you get page not found).

So I think it's now down to urls like civicrm/activity/fee/fi/fo/fum, where before the patch it would generate civicrm/activity/fee:fi:fo:fum and now it leaves it as civicrm/activity/fee/fi/fo/fum, but I don't know where this would matter. When you visit those links, regardless of how they're formatted, we're talking about links handled by some civi code. For it to matter there would need to be a(nother) drupal module that handles visits to some links starting with /civicrm.
If the link is generated on the drupal side somewhere, either with slashes or colons, then this PR doesn't change anything because this PR doesn't affect when you visit a link.