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.

Sign up for GitHub

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

#42438 wp_preload_links implementation #2505

Closed
wants to merge 13 commits into from
Closed

#42438 wp_preload_links implementation #2505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7e2f394
wp_preload_links implementation
manuelRod Apr 4, 2022
de8e6ea
Adding unit testing
manuelRod Apr 5, 2022
1f84c8f
fixing test
manuelRod Apr 5, 2022
d15930a
Adding unique urls check, more testing and small refactoring on checks
manuelRod Apr 6, 2022
8cabd1d
lint fix test file
manuelRod Apr 6, 2022
3bf691f
nitpicks
manuelRod Apr 11, 2022
551f0c5
nitpick
manuelRod Apr 11, 2022
8761430
nitpicks
manuelRod Apr 20, 2022
57d1232
Merge branch 'trunk' into 42438-add-support-preload
manuelRod May 11, 2022
b95d43d
renaming unique_atts
manuelRod May 11, 2022
bc134c6
refactor unit test with data provider.
manuelRod May 11, 2022
fd96232
fixing coding styles
manuelRod May 11, 2022
e1614b1
bumping release version
manuelRod May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/wp-includes/default-filters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@
add_action( 'wp_head', '_wp_render_title_tag', 1 );
add_action( 'wp_head', 'wp_enqueue_scripts', 1 );
add_action( 'wp_head', 'wp_resource_hints', 2 );
add_action( 'wp_head', 'wp_preload_links', 1 );
add_action( 'wp_head', 'feed_links', 2 );
add_action( 'wp_head', 'feed_links_extra', 3 );
add_action( 'wp_head', 'rsd_link' );
Expand Down
115 changes: 115 additions & 0 deletions src/wp-includes/general-template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3422,6 +3422,121 @@ function wp_resource_hints() {
}
}

/**
* Prints resource preloads directives to browsers.
*
* Gives directive to browsers to preload specific resources that website will
* need very soon, this ensures that they are available earlier and are less
* likely to block the page's render.
*
* These performance improving indicators work by using `<link rel"preload">`.
*
* @link https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types/preload
* @link https://web.dev/preload-responsive-images/
*
* @since 6.1.0
*/
function wp_preload_links() {
/**
* Filters domains and URLs for resource preloads.
*
* @since 6.1.0
*
* @param array $urls {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urls seems mis-named? maybe link_resources or link_attributes?

Copy link
Author

@manuelRod manuelRod May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 I see, poor naming choice but ok!

Copy link
Contributor

@azaozz azaozz Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* Array of resources and their attributes, or URLs to print for resource preloads.
*
* @type array ...$0 {
* Array of resource attributes.
manuelRod marked this conversation as resolved.
Show resolved Hide resolved
*
* @type string $href URL to include in resource preloads. Required.
* @type string $as How the browser should treat the resource
* (`script`, `style`, `image`, `document`, etc).
* @type string $crossorigin Indicates the CORS policy of the specified resource.
* @type string $type Type of the resource (`text/html`, `text/css`, etc).
* @type string $media Accepts media types or media queries. Allows responsive preloading.
* @type string $imagesizes Responsive source size to the source Set.
* @type string $imagesrcset Responsive image sources to the source set.
* }
* }
*/
$urls = apply_filters( 'wp_preload_links', array() );

if ( ! is_array( $urls ) ) {
return;
}

$unique_urls = array();
foreach ( $urls as $url ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to foreach ( $preload_resources as $resource )

if ( ! is_array( $url ) ) {
continue;
}

$atts = $url;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$atts means attributes, right? Why not use the whole word. Makes it a tiny bit easier to read :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

if ( isset( $url['href'] ) ) {
$url = $url['href'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed here

if ( isset( $unique_urls[ $url ] ) ) {
continue;
}
$unique_urls[ $url ] = $atts;
// Media can use imagesrcset and not href.
} elseif ( ! isset( $url['href'] )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() )?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed redundant ! isset()

&& ( 'image' === $url['as'] )
&& ( isset( $url['imagesrcset'] ) || isset( $url['imagesizes'] ) )
) {
if ( isset( $unique_urls[ $url['imagesrcset'] ] ) ) {
adamsilverstein marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
$unique_urls[ $url['imagesrcset'] ] = $atts;
} else {
continue;
}

foreach ( $unique_urls as $unique_atts ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, what's $unique_atts? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed this to foreach ( $unique_resources as $unique_resource ) {

$html = '';

foreach ( $unique_atts as $attr => $value ) {
/**
* Ignore not supported attributes.
*/
if ( ! is_scalar( $value )
manuelRod marked this conversation as resolved.
Show resolved Hide resolved
|| (
! in_array( $attr, array( 'as', 'crossorigin', 'href', 'imagesrcset', 'imagesizes', 'type', 'media' ), true )
&& ! is_numeric( $attr )
)
Comment on lines +3502 to +3505
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this can be split into several conditionals to make it easier to read? Seems it will be a lot better/simpler.

) {

continue;
}

// imagesrcset only usable when preloading image, ignore otherwise.
if ( ( 'imagesrcset' === $attr ) && ( ! isset( $unique_atts['as'] ) || ( 'image' !== $unique_atts['as'] ) ) ) {
continue;
}

// imagesizes only usable when preloading image and imagesrcset present, ignore otherwise.
if ( ( 'imagesizes' === $attr )
&& ( ! isset( $unique_atts['as'] ) || ( 'image' !== $unique_atts['as'] ) || ! isset( $unique_atts['imagesrcset'] ) )
) {
continue;
}

$value = ( 'href' === $attr ) ? esc_url( $value, array( 'http', 'https' ) ) : esc_attr( $value );

if ( ! is_string( $attr ) ) {
$html .= " $value";
manuelRod marked this conversation as resolved.
Show resolved Hide resolved
} else {
$html .= " $attr='$value'";
}
}
}

$html = trim( $html );

printf( "<link rel='preload' %s />\n", $html );
}

}

/**
* Retrieves a list of unique hosts of all enqueued scripts and styles.
*
Expand Down
251 changes: 251 additions & 0 deletions tests/phpunit/tests/general/wpPreloadLinks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,251 @@
<?php

/**
* @group general
* @group template
* @ticket 42438
* @covers ::wp_preload_links
*/
class Tests_General_wpPreloadLinks extends WP_UnitTestCase {
manuelRod marked this conversation as resolved.
Show resolved Hide resolved

/**
* @dataProvider provider_preload_links
*/
public function test_preload_links( $expected, $urls ) {
$callback = function () use ( $urls ) {
Copy link
Contributor

@azaozz azaozz Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed preload_resources

return $urls;
};

add_filter( 'wp_preload_links', $callback, 10 );
$actual = get_echo( 'wp_preload_links' );
remove_filter( 'wp_preload_links', $callback );

$this->assertSame( $expected, $actual );
}

/**
* Test provider for all preload link possible combinations.
*
* @return array[]
*/
public function provider_preload_links() {
return array(
'basic_preload' => array(
'expected' => "<link rel='preload' href='https://example.com/style.css' as='style' />\n",
'urls' => array(
array(
'href' => 'https://example.com/style.css',
'as' => 'style',
),
),
),
'multiple_links' => array(
'expected' => "<link rel='preload' href='https://example.com/style.css' as='style' />\n" .
"<link rel='preload' href='https://example.com/main.js' as='script' />\n",
'urls' => array(
array(
'href' => 'https://example.com/style.css',
'as' => 'style',
),
array(
'href' => 'https://example.com/main.js',
'as' => 'script',
),
),
),
'MIME_types' => array(
'expected' => "<link rel='preload' href='https://example.com/style.css' as='style' />\n" .
"<link rel='preload' href='https://example.com/video.mp4' as='video' type='video/mp4' />\n" .
"<link rel='preload' href='https://example.com/main.js' as='script' />\n",
'urls' => array(
array(
//Should ignore not valid attributes
'not' => 'valid',
'href' => 'https://example.com/style.css',
'as' => 'style',
),
array(
'href' => 'https://example.com/video.mp4',
'as' => 'video',
'type' => 'video/mp4',
),
array(
'href' => 'https://example.com/main.js',
'as' => 'script',
),
),
),
'CORS' => array(
'expected' => "<link rel='preload' href='https://example.com/style.css' as='style' crossorigin='anonymous' />\n" .
"<link rel='preload' href='https://example.com/video.mp4' as='video' type='video/mp4' />\n" .
"<link rel='preload' href='https://example.com/main.js' as='script' />\n" .
"<link rel='preload' href='https://example.com/font.woff2' as='font' type='font/woff2' crossorigin />\n",
'urls' => array(
array(
'href' => 'https://example.com/style.css',
'as' => 'style',
'crossorigin' => 'anonymous',
),
array(
'href' => 'https://example.com/video.mp4',
'as' => 'video',
'type' => 'video/mp4',
),
array(
'href' => 'https://example.com/main.js',
'as' => 'script',
),
array(
//Should ignore not valid attributes.
'ignore' => 'ignore',
'href' => 'https://example.com/font.woff2',
'as' => 'font',
'type' => 'font/woff2',
'crossorigin',
),
),
),
'media' => array(
'expected' => "<link rel='preload' href='https://example.com/style.css' as='style' crossorigin='anonymous' />\n" .
"<link rel='preload' href='https://example.com/video.mp4' as='video' type='video/mp4' />\n" .
"<link rel='preload' href='https://example.com/main.js' as='script' />\n" .
"<link rel='preload' href='https://example.com/font.woff2' as='font' type='font/woff2' crossorigin />\n" .
"<link rel='preload' href='https://example.com/image-narrow.png' as='image' media='(max-width: 600px)' />\n" .
"<link rel='preload' href='https://example.com/image-wide.png' as='image' media='(min-width: 601px)' />\n",
'urls' => array(
array(
'href' => 'https://example.com/style.css',
'as' => 'style',
'crossorigin' => 'anonymous',
),
array(
'href' => 'https://example.com/video.mp4',
'as' => 'video',
'type' => 'video/mp4',
),
// Duplicated href should be ignored.
array(
'href' => 'https://example.com/video.mp4',
'as' => 'video',
'type' => 'video/mp4',
),
array(
'href' => 'https://example.com/main.js',
'as' => 'script',
),
array(
'href' => 'https://example.com/font.woff2',
'as' => 'font',
'type' => 'font/woff2',
'crossorigin',
),
array(
'href' => 'https://example.com/image-narrow.png',
'as' => 'image',
'media' => '(max-width: 600px)',
),
array(
'href' => 'https://example.com/image-wide.png',
'as' => 'image',
'media' => '(min-width: 601px)',
),

),
),
'media_extra_attributes' => array(
'expected' => "<link rel='preload' href='https://example.com/style.css' as='style' crossorigin='anonymous' />\n" .
"<link rel='preload' href='https://example.com/video.mp4' as='video' type='video/mp4' />\n" .
"<link rel='preload' href='https://example.com/main.js' as='script' />\n" .
"<link rel='preload' href='https://example.com/font.woff2' as='font' type='font/woff2' crossorigin />\n" .
"<link rel='preload' href='https://example.com/image-640.png' as='image' imagesrcset='640.png 640w, 800.png 800w, 1024.png 1024w' imagesizes='100vw' />\n" .
"<link rel='preload' as='image' imagesrcset='640.png 640w, 800.png 800w, 1024.png 1024w' imagesizes='100vw' />\n" .
"<link rel='preload' href='https://example.com/image-wide.png' as='image' media='(min-width: 601px)' />\n" .
"<link rel='preload' href='https://example.com/image-800.png' as='image' imagesrcset='640.png 640w, 800.png 800w, 1024.png 1024w' />\n",
'urls' => array(
array(
'href' => 'https://example.com/style.css',
'as' => 'style',
'crossorigin' => 'anonymous',
),
array(
'href' => 'https://example.com/video.mp4',
'as' => 'video',
'type' => 'video/mp4',
),
array(
'href' => 'https://example.com/main.js',
'as' => 'script',
),
array(
'href' => 'https://example.com/font.woff2',
'as' => 'font',
'type' => 'font/woff2',
'crossorigin',
),
// imagesrcset only possible when using image, ignore.
array(
'href' => 'https://example.com/font.woff2',
'as' => 'font',
'type' => 'font/woff2',
'imagesrcset' => '640.png 640w, 800.png 800w, 1024.png 1024w',
),
// imagesizes only possible when using image, ignore.
array(
'href' => 'https://example.com/font.woff2',
'as' => 'font',
'type' => 'font/woff2',
'imagesizes' => '100vw',
),
// Duplicated href should be ignored.
array(
'href' => 'https://example.com/font.woff2',
'as' => 'font',
'type' => 'font/woff2',
'crossorigin',
),
array(
'href' => 'https://example.com/image-640.png',
'as' => 'image',
'imagesrcset' => '640.png 640w, 800.png 800w, 1024.png 1024w',
'imagesizes' => '100vw',
),
// Omit href so that unsupporting browsers won't request a useless image.
array(
'as' => 'image',
'imagesrcset' => '640.png 640w, 800.png 800w, 1024.png 1024w',
'imagesizes' => '100vw',
),
// Duplicated imagesrcset should be ignored.
array(
'as' => 'image',
'imagesrcset' => '640.png 640w, 800.png 800w, 1024.png 1024w',
'imagesizes' => '100vw',
),
array(
'href' => 'https://example.com/image-wide.png',
'as' => 'image',
'media' => '(min-width: 601px)',
),
// No href but not imagesrcset, should be ignored
array(
'as' => 'image',
'media' => '(min-width: 601px)',
),
// imagesizes is optional
array(
'href' => 'https://example.com/image-800.png',
'as' => 'image',
'imagesrcset' => '640.png 640w, 800.png 800w, 1024.png 1024w',
),
// imagesizes should be ignored since imagesrcset not present.
array(
'href' => 'https://example.com/image-640.png',
'as' => 'image',
'imagesizes' => '100vw',
),
),
),
);
}

}