#56231 closed defect (bug) (fixed)
WP_Http::make_absolute_url drops fragments
Reported by: |
|
Owned by: |
|
---|---|---|---|
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.
11 months ago
#1
- Keywords has-patch has-unit-tests added
#2
@
11 months 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?
#3
@
10 months 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.
#4
@
10 months 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 offalse
alleviate your concerns?
#5
@
10 months 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
@
8 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.
4 months ago
#8
@
3 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.
3 months ago
@peterwilsoncc commented on PR #2983:
3 months ago
#11
#12
@
3 months ago
Added in Misc dev note. Draft: https://make.wordpress.org/core/?p=103089&preview=1&_ppp=36765ffd5f
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