-
Notifications
You must be signed in to change notification settings - Fork 138
Avoid passing positional parameters in Optimization Detective #1338
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
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 |
adamsilverstein
left a comment
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_visitorsaction. At the moment, this action is passed instances of the following three classes as positional parameters:OD_Tag_Visitor_RegistryOD_URL_Metrics_Group_CollectionOD_Link_CollectionIn 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_ProcessorOD_URL_Metrics_Group_CollectionOD_Link_CollectionAgain, when writing a tag visitor and you need to use the
OD_Link_Collectioninstance to add a link to the head, you have to skip over an unusedOD_URL_Metrics_Group_Collectioninstance:Not only is the
$url_metrics_group_collectionnot 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_visitorssince 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.