Make WordPress Core

Opened 2 years ago

Closed 17 months ago

Last modified 17 months ago

#56231 closed defect (bug) (fixed)

WP_Http::make_absolute_url drops fragments

Reported by: dshanske's profile dshanske Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.2 Priority: normal
Severity: normal Version: 3.4
Component: HTTP API Keywords: has-patch has-unit-tests dev-feedback commit needs-dev-note
Focuses: Cc:

Description

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.

Change History (12)

This ticket was mentioned in PR #2983 on WordPress/wordpress-develop by costdev.


2 years ago
#1

  • Keywords has-patch has-unit-tests added

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

#2 @costdev
2 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 6.1

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?

Last edited 2 years ago by costdev (previous) (diff)

#3 @schlessera
2 years ago

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

Last edited 2 years ago by schlessera (previous) (diff)

#4 @costdev
2 years ago

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?

#5 @peterwilsoncc
2 years ago

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.

#6 @desrosj
22 months ago

  • Milestone changed from 6.1 to 6.2

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.

This ticket was mentioned in Slack in #core by costdev. View the logs.


17 months ago

#8 @peterwilsoncc
17 months ago

  • Keywords commit needs-dev-note added
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

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.

This ticket was mentioned in Slack in #core by costdev. View the logs.


17 months ago

#10 @peterwilsoncc
17 months ago

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

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.

Note: See TracTickets for help on using tickets.