-
Notifications
You must be signed in to change notification settings - Fork 110
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
Avoid passing positional parameters in Optimization Detective #1338
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
a4f99d3
to
ce07f44
Compare
ce07f44
to
9662ed6
Compare
9662ed6
to
74ac1bb
Compare
… add/tag-visitor-context * 'trunk' of https://github.com/WordPress/performance: Add missing covers tags Pass explicit param to indicate whether processing fragment or entire document Improve copy Add note explaining why require_once is in function Add preconnects for the most popular embeds
The base PR has been merged so this is ready to go! |
See also proposed usage in the auto-sizes plugin: https://github.com/WordPress/performance/pull/1322/files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nifty!
This fixes something that has been bugging me for awhile.
The way that you register new tag visitors for Optimization Detective is via the
od_register_tag_visitors
action. At the moment, this action is passed instances of the following three classes as positional parameters:OD_Tag_Visitor_Registry
OD_URL_Metrics_Group_Collection
OD_Link_Collection
In reality, only the first parameter is needed. The other two are extras for convenience. Nevertheless, passing them as positional parameters means that developers have to remember which one is which, and when new possible classes are introduced, they just get added on to the end. The situation is similar for the tag visitor, as this callable is passed instances of the following classes:
OD_HTML_Tag_Processor
OD_URL_Metrics_Group_Collection
OD_Link_Collection
Again, when writing a tag visitor and you need to use the
OD_Link_Collection
instance to add a link to the head, you have to skip over an unusedOD_URL_Metrics_Group_Collection
instance:Not only is the
$url_metrics_group_collection
not used here, but the possible strict type hints on the tag visitor callable mean that these positional parameters would be locked in stone forever. There would not be any flexibility to reorder the parameters or change the names of the classes.These issues can be fixed by introducing a context object that has these class instances exposed as properties. Compare the above with the below:
The benefits here are:
Similarly, there is no need for the additional parameters being passed to the
od_register_tag_visitors
since the parameters are passed as part of the context anyway. So they can just be eliminated entirely. They are not needed by Image Prioritizer or Embed Optimizer.