WP_Http::make_absolute_url drops fragments

[dshanske]

The purpose of this function is to make a URL absolute. It should not drop fragments for that reason as that is contrary to the purpose of the function.

WP_Http::make_absolute_url() does not return URLs including the original fragment(s) i.e. http://example.org/#a_fragment.

This PR ensures the return URL includes its original fragment(s). Unit tests included.

Trac ticket: https://core.trac.wordpress.org/ticket/56231

[costdev]

Hi @dshanske, thanks for opening this ticket.

I have opened ​PR 2983 to address this issue.

@jrf @schlessera, do you have any concerns about this ticket's request, or about the implementation in ​PR 2983? For example, it may be preferable to add a new (bool) $include_fragments = false argument to maintain BC, possibly?

[schlessera]

@costdev For acting on the URLs from the Requests side, @jrf and I see no issue. Requests handles fragments just fine, and it doesn't care too much about whether you use them or not.

The only issues I could see is from WordPress Core or plugins that don't expect URLs to contain fragments because they were historically missing. They might have a use for the $include_fragments flag, but then again, if you keep it false by default, you don't actually fix the bug, and if you keep it true by default, they will need to make a change to their code anyway - they could just as well fix the actual bug then.

[costdev]

Thanks for the feedback @schlessera and @jrf!

puts hand into The Big Hat of Committer Names... @SergeyBiryukov @audrasjb @peterwilsoncc

• Do any of you have concerns about ​including fragments in the URLs by default to resolve this bug?
• Are there any existing uses where including fragments could produce an error? If so, would adding an $include_fragments argument with a default value of false alleviate your concerns?

[peterwilsoncc]

A ​search of the plugin repo for make_absolute_url shows 88 plugins include the string. The most popular plugin (700K installs) is a documentation reference to this very bug :)

Most of the plugins over 1000 installs seem to be false positives; there are one or two that are genuine but I don't think they're affected (mainly because they use url_to_postid() which drops fragments).

My inclination is to switch to returning the fragment, document the BC break in a dev note and refer people to strip_fragment_from_url() if they were relying on the URL without a fragment.

[desrosj]

Because this one needs more discussion and the suggestion was made to introduce a breaking change and use dev notes to communicate to the community, I think it's best to retry this one in 6.2.

If another committer feels differently and is willing to own this one for 6.1, it can be moved back.

[peterwilsoncc]

The linked pull request looks ready for commit.

I've added a dev-note requirement to alert developers to include strip_fragment_from_url() if they wish to maintain the legacy behaviour. This can be included in the miscellaneous developer related changes note.

[peterwilsoncc]

In 55370:

HTTP API: Add fragment support to WP_Http::make_absolute_url().

Modifies WP_Http::make_absolute_url() to prevent it from dropping URL fragments, this in turn fixes the same issue for links_add_base_url().

Props costdev, sergeybiryukov, dshanske, schlessera, jrf, desrosj, dd32.
Fixes #56231.

Committed in r55370 / ​https://github.com/WordPress/wordpress-develop/commit/072274690b52cfa0c5e5da763e70a7af24f00da7

[milana_cap]

Added in Misc dev note. Draft: ​https://make.wordpress.org/core/?p=103089&preview=1&_ppp=36765ffd5f