Make WordPress Core

Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#44499 closed defect (bug) (fixed)

add_query_arg function doesn't remove the empty '?' when an anchor exists

Reported by: benjaminanakena's profile benjamin.anakena Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: minor Version: 4.9.6
Component: HTTP API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

To reproduce:

<?php
$url = 'https://domain.com/uri?param=value#anchor';
$url = add_query_arg( 'param', false, $url );
echo $url;

Expected value: https://domain.com/uri#anchor
Returned value: https://domain.com/uri?#anchor

Attachments (3)

44499.patch (414 bytes) - added by sabernhardt 3 years ago.
replace ?# with #
44499.1.diff (1.2 KB) - added by costdev 2 years ago.
Unit test added.
44499-test-before-after.gif (5.9 MB) - added by hellofromTonya 2 years ago.
Browser Test Report: same behavior before and after 44499.1.diff. No regression. Works before and after.

Change History (15)

@sabernhardt
3 years ago

replace ?# with #

#1 @sabernhardt
3 years ago

  • Keywords has-patch needs-testing added

@benjaminanakena Thanks for the report!

44499.patch works with the example.

#2 @sabernhardt
3 years ago

#52503 was marked as a duplicate.

Version 0, edited 3 years ago by sabernhardt (next)

#3 @sabernhardt
2 years ago

  • Milestone changed from Awaiting Review to 5.9
  • Owner set to sabernhardt
  • Status changed from new to accepted

#4 @hellofromTonya
2 years ago

  • Keywords needs-unit-tests added
  • Owner changed from sabernhardt to hellofromTonya
  • Status changed from accepted to assigned

@costdev
2 years ago

Unit test added.

#5 @costdev
2 years ago

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

This ticket was mentioned in PR #1902 on WordPress/wordpress-develop by hellofromtonya.


2 years ago
#6

Applies patch 44499.1.diff.
Adds more test datasets.

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

@hellofromTonya
2 years ago

Browser Test Report: same behavior before and after 44499.1.diff. No regression. Works before and after.

#7 @hellofromTonya
2 years ago

  • Keywords needs-testing removed

Test Report

Env:

  • Browser: Chrome, Firefox, Edge, and Safari
  • OS: macOS
  • localhost: wp-env
  • WordPress: trunk
  • Theme: TT2
  • Plugins: none

Steps

  1. Go to "Posts" and then "Add New"
  2. In the content, add some filler paragraphs.
  3. And then add a header block towards the bottom of the content.
  4. With the header block selected, click on "Advanced" and set the "HTML anchor" to anchor.
  5. Set the permalinks to something other than "Plain".
  6. View the new post in the front-end
  7. Click in the browser's search bar and append ?param=value#anchor to the end of the URL. Press enter or return.

The post should reload and jump to the header block.

  1. Apply the patch.
  2. Repeat step 6 and 7.

Should again jump to the header block.

  1. Repeat steps 6 to 7 using by appending this after the URL: ?#anchor.

Should again jump to the header block.

Results

There is no bug or issue in the front-end as the browser handles the query args and jumps to the anchor ✅

With and without the patch, each browser behaved the same ✅

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

#8 @hellofromTonya
2 years ago

  • Keywords commit added

While the add_query_arg() does not remove the unneeded ? when there are no query args, it works in each browser tested. Granted it seems odd to leave ? when it's not needed. But I wonder: Is there's a browser or device where this does not work?

For backend correctness, it makes sense to remove the unneeded ? character. This can help with further processing where that character is not expected and guard Core against changes in browsers or incompatibility between apps and browsers.

PR 1902 is ready for commit.

#9 @hellofromTonya
2 years ago

  • Status changed from assigned to reviewing

Preparing commit.

#10 @hellofromTonya
2 years ago

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

In 52187:

HTTP API: Remove empty ? when only anchor remains in add_query_arg().

If after processing through add_query_arg() a ?# remains, this commit removes the unnecessary and unused ? character as there are no query args in the URL.

Includes tests.

Follow-up to [1823], [5193], [5999], [6005].

Props benjaminanakenam, sabernhardt, costdev, hellofromTonya.
Fixes #44499.

#11 @hellofromTonya
2 years ago

  • Component changed from General to HTTP API

Thank you everyone for contributing! The patch has been committed and will ship in 5.9.

Note: See TracTickets for help on using tickets.