Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#51839 closed defect (bug) (fixed)

When get_feed_permastruct() return false, get_feed_link() return incorrect non feed url

Reported by: hauvong's profile hauvong Owned by: johnbillion's profile johnbillion
Milestone: 5.7 Priority: normal
Severity: minor Version: 5.5
Component: Feeds Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

When $this->permalink_structure is empty & $this->feed_structure is not set, the function get_feed_permastruct() would return boolean false causing get_feed_link() return incorrect non-feed url.

ref: https://github.com/WordPress/wordpress-develop/blob/4f28c68eb61c656ba6ee3305ee066ff76d7e7403/src/wp-includes/link-template.php#L628

Patch file to fix is attached.

Attachments (3)

class-wp-rewrite.php.patch (556 bytes) - added by hauvong 4 years ago.
51839.diff (534 bytes) - added by peterwilsoncc 4 years ago.
51839.2.diff (2.6 KB) - added by peterwilsoncc 4 years ago.

Download all attachments as: .zip

Change History (11)

#1 @peterwilsoncc
4 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.7

Thanks for the report, I've verified the issue. It was introduced in [47808] during the 5.5 release.

I'll move this to the 5.7 milestone. It would be good to include some tests for get_feed_link() when the permalink structure is undefined so I've added the needs-unit-tests keyword too.

@peterwilsoncc
4 years ago

#2 @peterwilsoncc
4 years ago

In 51839.diff I've moved the change to get_feed_link() rather than changing the return value and type of $wp_rewrite->get_feed_permastruct().

This is to avoid backward compatibility breaking for theme or plugin developers using the condition if ( $wp_rewrite->get_feed_permastruct() === false ) {}.

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


4 years ago

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


4 years ago

#5 @peterwilsoncc
4 years ago

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

In 51839.2.diff :

  • Same change to get_feed_link() as earlier, using a truthy check rather than strict check
  • Unit tests x3 sets
    • plain permalinks
    • pretty links without a plain text prefix
    • pretty links with a plain text prefix

All looks to be passing but another set of eyes would be ideal.

#6 @SergeyBiryukov
4 years ago

  • Keywords commit added

Thanks @hauvong for catching the issue and @peterwilsoncc for the tests!

51839.2.diff looks good to me.

#7 @johnbillion
4 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

Agreed and the tests fail as expected if the strict check is reintroduced.

#8 @johnbillion
4 years ago

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

In 50354:

Feeds: Fix the URL returned by get_feed_link() when pretty permalinks are not in use.

Props hauvong, peterwilsoncc, SergeyBiryukov

Fixes #51839

Note: See TracTickets for help on using tickets.