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

Remove static caching of static on Contribution page tab #15329

Merged

Conversation

mattwire
Copy link
Contributor

Overview

This one is for @eileenmcnaughton ... Remove the static caching on a static function.

Before

Unnecessary code, less comments

After

Cleaner code, comments/docblocks cleaned up

Technical Details

This cleans up an early return + static caching of the recurLinks() function which is already static.

Comments

@civibot
Copy link

civibot bot commented Sep 19, 2019

(Standard links)

@civibot civibot bot added the master label Sep 19, 2019
@mattwire mattwire force-pushed the contributepagetab_cleanupstatic branch from b6527e9 to 04d1551 Compare September 19, 2019 15:10
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@totten
Copy link
Member

totten commented Sep 20, 2019

I think I like this, but the rationale ("unnecessary") seemed hand-wavy. Trying to read between the lines...

Caching that initial array of 3 elements looks a lot like a micro-optimization. The micro-optimization might make sense if, hypothetically, ::recurLinks() was called 10,000 times in some pageview. But that hypothetical seems only hypothetical; and even if that use-case is conceded, the expected gain doesn't bear-out in benchmarking on php7. (Make a throw-away script which calls recurLinks() 10k times and compare with/without $_links).

+1 for simpler code

@mattwire
Copy link
Contributor Author

@totten Thanks :-) @eileenmcnaughton particularly doesn't like the static $_links, I didn't mind. But it does simplify the code without!

@mattwire
Copy link
Contributor Author

mattwire commented Oct 4, 2019

@eileenmcnaughton Please review comments and merge if you are happy?

@mattwire mattwire added the merge ready PR will be merged after a few days if there are no objections label Oct 4, 2019
@eileenmcnaughton
Copy link
Contributor

I worked through the refactor & it seems fine - links still exist on the recurring contribution tab

@eileenmcnaughton eileenmcnaughton merged commit 6698aea into civicrm:master Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants