WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#37240 closed defect (bug) (fixed)

Remove schemes from dns-prefetch resource hint outputs

Reported by: niallkennedy Owned by: voldemortensen
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: General Keywords: has-patch has-unit-tests commit
Focuses: performance Cc:
PR Number:

Description

Remove URL scheme from dns-prefetch link relation outputs.

The following dns-prefetch array values should all have the same DNS lookup:

  • wordpress.org
  • http://wordpress.org
  • https://wordpress.org

should either remove duplicate entries after extracting only the host component of URLs or expect anyone acting on the filter to use only a host component.

Attachments (2)

37240.diff (2.4 KB) - added by niallkennedy 4 years ago.
remove scheme from dns-prefetch output
37240.2.diff (5.0 KB) - added by peterwilsoncc 4 years ago.

Download all attachments as: .zip

Change History (13)

@niallkennedy
4 years ago

remove scheme from dns-prefetch output

#1 @niallkennedy
4 years ago

  • Keywords has-patch has-unit-tests added

#2 @ocean90
4 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to ocean90
  • Status changed from new to reviewing
  • Type changed from enhancement to defect (bug)

Thanks for the report @niallkennedy!

@voldemortensen Any thoughts on that?

#3 @ocean90
4 years ago

  • Owner changed from ocean90 to voldemortensen

#4 @voldemortensen
4 years ago

My initial thoughts are this is a great improvement. But then I thought some more. I don't know how browsers are implementing this. If I were implementing dns-prefetch, I'd ignore the scheme altogether because the scheme has no effect on DNS. However, I didn't do it, so I don't know. It's probably more safe to err on the side of caution here to avoid unnecessary CPU usage/bandwidth/etc. So +1 for dns-prefetching only.

Theoretically:

<link rel="prerender" href="http://site.com/post-name">
<link rel="prerender" href="https://site.com/post-name">

can be different, so we should probably leave those alone unless we want to try and keep people from doing unintended things.

#5 @voldemortensen
4 years ago

  • Keywords commit added

#6 @swissspidy
4 years ago

  • Keywords needs-unit-tests added; has-unit-tests commit removed

The specification isn't clear about the schemes and uses protocol-relative URLs or relative URLs everywhere in the examples.

For Chromium, it doesn't matter whether it's // or http://:

The double slashes indicate that the URL starts with a host name (as specified in RFC 1808). It is equivalent (but unnecessary) to use a full URL such as "http://host_name_to_prefetch.com/".

Source: https://www.chromium.org/developers/design-documents/dns-prefetching

That's also the impression I got when looking at the Webkit source code.

The only benefit I see is that we'd be able to filter out duplicates (which the browser would probably do anyway). As per the ticket description, wordpress.org, http://wordpress.org, https://wordpress.org should all have the same DNS lookup and therefore only need 1 entry.

We need a unit test to cover this.

#7 @peterwilsoncc
4 years ago

In 37240.2.diff:

  • minor shuffling to apply array_unique following tidyup
  • unit tests to ensure equivalent resources are combined for dns-prefetch
  • replaced the \r\n with \n because it's not required

#8 @peterwilsoncc
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#9 @ocean90
4 years ago

  • Keywords commit added

37240.2.diff looks good to me.

#10 @niallkennedy
4 years ago

diff 2 is an improvement avoiding duplication.

The $urls value is reused but bad values might not be removed during the first for loop. If the result of esc_url returns an empty string or if the host value of the parsed URL is empty then the array value should not be output.

#11 @ocean90
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 38036:

Resource Hints: Remove schemes from dns-prefetch resource hint outputs.

"wordpress.org", "http://wordpress.org", and "https://wordpress.org" should all have the same DNS lookup.
Also, replace \r\n with \n and ensure that invalid URLs are skipped.

Props niallkennedy, peterwilsoncc.
Fixes #37240.

Note: See TracTickets for help on using tickets.