-
-
Notifications
You must be signed in to change notification settings - Fork 823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev/drupal#200: Performance improvement #31634
base: master
Are you sure you want to change the base?
Conversation
Performance improvement
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/drupal/-/issues/200 |
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:
after the patch:
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. |
@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 |
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. |
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