Skip to content
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

Generate URLs faster - another attempt #98

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

demeritcowboy
Copy link
Contributor

Another attempt at #88

This one seems to work with the edge cases we were having trouble with:

  • cache clear
  • weirdo urls like civicrm/report/instance/14 where the parameter is part of the path
  • oddballs like the civicrm/upgrade queue runner

I have not yet tested the installer or done any benchmarking on speed. Going to do that next.

There's pros and cons to using Civi::cache instead of Civi::statics for storing the menu items, which is the bulk of the problem. It seems to work with either but at the moment I've used cache.
For the known paths, I'm not sure we need to even cache those if we're caching the menu build, but I've left that in.

@MegaphoneJon @jackrabbithanna

@demeritcowboy
Copy link
Contributor Author

I tested the installer - ok.
For speed tests, with a sample size of 1, I'm not too impressed with the speed improvement. Will need to do more benchmarking. I checked and it is hitting the cache, and the number of items is the same with or without the cache, so it seems to be working as intended.

I'm not worried about the jenkins fails here. Pretty sure they're known issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant