#42438 wp_preload_links implementation #2505
Conversation
There was a problem hiding this comment.
Left a couple of comments. Overall looks quite nice.
But I would recommend also checking out WordPress/performance#31 before investing much more time into this.
One concern raised there was that providing this simply API doesn't provide particularly useful.
|
@swissspidy thanks for the feedback, I've addressed it. I also interacted on the performance issue, but I did not get any feedback from there. |
|
@manuelRod left some small feedback/questions on the PR |
|
@adamsilverstein thanks for the feedback, I left also some answers |
| * | ||
| * @since 6.0.0 | ||
| * | ||
| * @param array $urls { |
There was a problem hiding this comment.
urls seems mis-named? maybe link_resources or link_attributes?
There was a problem hiding this comment.
@adamsilverstein I used $url the same way wp_resource_hints uses it. Since both share a similar structure/naming, I didn't want to introduce many changes.
There was a problem hiding this comment.
👍🏼 I see, poor naming choice but ok!
There was a problem hiding this comment.
I used $url the same way wp_resource_hints uses it.
I see. However imho in both places the name is no good, unfortunately. On top of that further down the var $url is reset/reused. Not sure that's a good choice, perhaps better to have different names?
IMHO, best to make the names here descriptive, self-documenting and also fix the confusing naming in wp_resource_hints. Thinking both of @adamsilverstein's suggestions are better, maybe link_attributes makes a bit more sense.
There was a problem hiding this comment.
since we are renaming the function slightly based on your last comment to wp_preload_resources, thinking the variable name would be $preload_resources. How does that sound?
manuelRod
changed the title
|
@adamsilverstein @swissspidy @spacedmonkey all ready here |
|
Please update on references to 6.0.0 and change them to 6.1.0. |
|
This feels like it is very close. |
little refactor (printf instead of echo)
|
@spacedmonkey references version bumped |
|
|
||
| $atts = $url; | ||
| if ( isset( $url['href'] ) ) { | ||
| $url = $url['href']; |
There was a problem hiding this comment.
Don't think resetting/reusing vars is a good idea, ever. I know, it comes from wp_resource_hints() however it should be fixed there rather than replicated here. See above.
| } | ||
| $unique_urls[ $url ] = $atts; | ||
| // Media can use imagesrcset and not href. | ||
| } elseif ( ! isset( $url['href'] ) |
There was a problem hiding this comment.
The first condition seems redundant? We already know it is an array, so if an element is isset() what's the point in doing elseif ( ! isset() )?
There was a problem hiding this comment.
removed redundant ! isset()
| continue; | ||
| } | ||
|
|
||
| $atts = $url; |
There was a problem hiding this comment.
$atts means attributes, right? Why not use the whole word. Makes it a tiny bit easier to read :)
| continue; | ||
| } | ||
|
|
||
| foreach ( $unique_urls as $unique_atts ) { |
There was a problem hiding this comment.
changed this to foreach ( $unique_resources as $unique_resource ) {
| || ( | ||
| ! in_array( $attr, array( 'as', 'crossorigin', 'href', 'imagesrcset', 'imagesizes', 'type', 'media' ), true ) | ||
| && ! is_numeric( $attr ) | ||
| ) |
There was a problem hiding this comment.
Perhaps this can be split into several conditionals to make it easier to read? Seems it will be a lot better/simpler.
| * @dataProvider provider_preload_links | ||
| */ | ||
| public function test_preload_links( $expected, $urls ) { | ||
| $callback = function () use ( $urls ) { |
There was a problem hiding this comment.
As above, seems $urls is not a good descriptive name for this. It doesn't contain just URLs, right? link_attributes would probably be better.
There was a problem hiding this comment.
renamed preload_resources
|
@manuelRod 👋🏼 I'm going to pick this up and add the recommended feedback above to help move this ticket forward. |
| } | ||
|
|
||
| $unique_urls = array(); | ||
| foreach ( $urls as $url ) { |
There was a problem hiding this comment.
changed to foreach ( $preload_resources as $resource )
|
@manuelRod - I was unable to push changes here, I don't have permissions. So instead I opened a new PR on my fork - #3041; happy to close that and push changes here instead if you can give me that capability, otherwise will continue work there. If you review the last commit(s) you will see the fixes from the feedback above. |
Trac ticket: https://core.trac.wordpress.org/ticket/42438
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.