Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#35084 closed defect (bug) (fixed)

check for post status in get_page_uri causes issues with permalinks

Reported by: tharsheblows's profile tharsheblows Owned by: netweb's profile netweb
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Permalinks Keywords: fixed-major
Focuses: Cc:

Description

The check for 'publish' here [34001] is causing permalinks to be incorrect if post ancestors have a custom post status, eg 'private' or 'closed' in bbPress -- it skips them in making the permalink and they need to be in there.

As the original issue was with trashed posts, the patch just makes sure post_status isn't trash and the unit test still passes.

Attachments (5)

35084.diff (425 bytes) - added by tharsheblows 9 years ago.
change post status to check that it's not in the trash
35084.2.diff (1.7 KB) - added by tharsheblows 9 years ago.
exclude post statuses with 'internal'=>true and add unit test
35084.3.diff (2.0 KB) - added by netweb 9 years ago.
35084.4.diff (977 bytes) - added by tharsheblows 9 years ago.
only ignore parent in permalink if it is not in database
35084.5.diff (1.3 KB) - added by tharsheblows 9 years ago.
only ignore parent if not in database (with docs this time)

Download all attachments as: .zip

Change History (30)

@tharsheblows
9 years ago

change post status to check that it's not in the trash

This ticket was mentioned in Slack in #bbpress by tharsheblows. View the logs.


9 years ago

#2 @swissspidy
9 years ago

  • Keywords has-patch needs-unit-tests added

What about other post statuses like inherit? I feel like get_post_stati() should be used.

A bit worried that the unit test still pass, I suggest creating new unit tests for different post statuses.

#3 follow-up: @tharsheblows
9 years ago

Do you mean test that all the other post statuses work correctly? I can do that in the morning, I haven't done many unit tests but would love to give it a go. :)

Or do you mean change the patch to omit other post statuses when building the permalink?

#4 in reply to: ↑ 3 @swissspidy
9 years ago

Replying to tharsheblows:

Do you mean test that all the other post statuses work correctly? I can do that in the morning, I haven't done many unit tests but would love to give it a go. :)

Or do you mean change the patch to omit other post statuses when building the permalink?

Both? :-)

I guess we can do something like ! in_array( $parent->post_status, get_post_stati( array( 'private' => true ) ) ) (if I'm not overthinking this).

Needs unit tests to ensure this works correctly for all desired post types.

#5 @tharsheblows
9 years ago

It would be completely hypocritical for me to judge anyone overthinking anything! ;)

'private'=>true won't work because one of the bbPress post statuses is registered with it, used on the front end and needs to be in the uri. However, 'internal'=>true should always work. This picks up 'inherit', 'auto-draft' and 'trash' which I _think_ are safe to exclude.

I've added in a unit test which iterates over the post statuses. I didn't replace the one in [34001] which checks differently, but I am not sure of the protocol there.

@tharsheblows
9 years ago

exclude post statuses with 'internal'=>true and add unit test

#6 @swissspidy
9 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from Awaiting Review to 4.4.1

Sweet, thanks for the unit tests!

Moving to 4.4.1 milestone since this is a regression.

#7 @mynamevenu24
9 years ago

'internal'=>true and add unit test

#8 @netweb
9 years ago

Attachment 35084.2.diff looks good from my perspective for both the patch and tests.

Some @since docs should be added for recent changes for the developer changelog reference, adding @since 4.4.0 docs for [34001], and then do we add @since 4.4.1 for the proposed change here?


I've added some test to bbPress in changeset:5962 that adds some assertions for bbPress' forum custom post type private and hidden posts statuses with hierarchal nested forum URL's e.g:

These assertions are failing for /trunk and 4.4 branches, with 35084.2.diff applied these assertions pass.

See https://travis-ci.org/ntwb/bbPress/builds/96968246

#9 follow-up: @tharsheblows
9 years ago

I don't know where the @since would go because nothing has really been added. I think as long as you put get_page_uri in the visible part of the commit message it should be ok? Or I don't know, it would for me, that's usually how I find things.

#10 in reply to: ↑ 9 ; follow-up: @netweb
9 years ago

Replying to tharsheblows:

I don't know where the @since would go because nothing has really been added. I think as long as you put get_page_uri in the visible part of the commit message it should be ok? Or I don't know, it would for me, that's usually how I find things.

In the PHPDoc's is what I'm referring to, e.g:

  • src/wp-includes/post.php

     
    42864286 * Sub pages will be in the "directory" under the parent page post name.
    42874287 *
    42884288 * @since 1.5.0
     4289 * @since 4.4.0 Do not add parent slugs to orphaned pages
     4290 * @since 4.4.1 Support custom post statuses
    42894291 *
    42904292 * @param WP_Post|object|int $page Page object or page ID.
    42914293 * @return string|false Page URI, false on error.
     
    43004302
    43014303        foreach ( $page->ancestors as $parent ) {
    43024304                $parent = get_post( $parent );
    4303                 if ( 'publish' === $parent->post_status ) {
     4305                if ( ! in_array( $parent->post_status, get_post_stati( array( 'internal' => true ) ) ) ){
    43044306                        $uri = $parent->post_name . '/' . $uri;
    43054307                }
    43064308        }

I'm not sure if we do the minor version for our docs, i.e. the 4.4.1 line in this case

#11 @netweb
9 years ago

  • Owner set to netweb
  • Status changed from new to assigned

#12 in reply to: ↑ 10 ; follow-up: @DrewAPicture
9 years ago

Replying to netweb:

I'm not sure if we do the minor version for our docs, i.e. the 4.4.1 line in this case

If it was important enough to change/fix in a point release, it qualifies for a changelog entry. The normal meter for a changelog entry is "significant changes" to functionality or logic. For more on the basis of changelogs, see the inline docs standard.

@netweb
9 years ago

#13 in reply to: ↑ 12 ; follow-up: @netweb
9 years ago

  • Keywords commit added

Patch 35084.3.diff includes PHPDoc @since references for devhub changelog

Replying to DrewAPicture:

If it was important enough to change/fix in a point release, it qualifies for a changelog entry. The normal meter for a changelog entry is "significant changes" to functionality or logic. For more on the basis of changelogs, see the inline docs standard.

Thanks Drew :) (Maybe a footnote in those docs explaining that would help the next person with same question)

#14 in reply to: ↑ 13 @DrewAPicture
9 years ago

Replying to netweb:

Patch 35084.3.diff includes PHPDoc @since references for devhub changelog

Replying to DrewAPicture:

If it was important enough to change/fix in a point release, it qualifies for a changelog entry. The normal meter for a changelog entry is "significant changes" to functionality or logic. For more on the basis of changelogs, see the inline docs standard.

Thanks Drew :) (Maybe a footnote in those docs explaining that would help the next person with same question)

Not sure I understand what's missing. There's an entire paragraph on what qualifies as a "significant change" :-)

#15 @tharsheblows
9 years ago

Thank you both! I hadn't looked at the change the right way - the discussion and link to the inline docs standard are super helpful. I wanted to use a smiley face emoji but it didn't work so :) will have to do.

Last edited 9 years ago by tharsheblows (previous) (diff)

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


9 years ago

#17 @dd32
9 years ago

35084.3.diff here does indeed fix the issue as provided here, that non-publish public statuses are not being added to URI's.
However, I think [34001] should be reverted.

Previously, a URI such as /page/trashed_page/child/ was the returned URI, and was the URI which the page could be accessed on.
Now, the URI will be returned as /page/child/ and will 404, however, /page/trashed_page/child/ will continue to return the page as expected.

I think the original issue in #15963 was that pages that were removed, ie. database row removed, would cause a PHP Notice.

#18 @tharsheblows
9 years ago

Ah, I see what you're saying. In the unit test in [34001] it seems to imply that a parent with a post_status of 'trash' should not be in the permalink but I was not part of that discussion so I'm not sure if that's correct.

The patch here simply checks if the parent exists and updates the unit test to completely remove the parent rather than change the post_status to trash.

This is causing issues in bbPress (and maybe BuddyPress?) so it would be great if this could be in 4.4.1 - reverting [34001] would sort it and any of the diffs should work

@tharsheblows
9 years ago

only ignore parent in permalink if it is not in database

@tharsheblows
9 years ago

only ignore parent if not in database (with docs this time)

#19 @netweb
9 years ago

Nice, thanks for looking and explaining @dd32 and iterating @tharsheblows

#20 @jorbin
9 years ago

@dd32 can you take a look at the latest patch? @wonderboymusic may also have some thoughts as he did the original commit.

#21 @dd32
9 years ago

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

In 36094:

Pages: get_page_uri() should return the URI at which the resource being accessed is available at, this may include non-'publish' status posts.

Reverts [34001] and fixes the original issue in #15963 - avoiding a PHP Notice for when the post doesn't exist.

Props tharsheblows.
See #15963.
Fixes #35084.

#22 @dd32
9 years ago

  • Keywords fixed-major added; has-patch has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Note that I left the changelog out, the changes between 4.3.x and 4.4.1 are minimal.

#23 @dd32
9 years ago

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

In 36105:

Pages: get_page_uri() should return the URI at which the resource being accessed is available at, this may include non-'publish' status posts.

Reverts [34001] and fixes the original issue in #15963 - avoiding a PHP Notice for when the post doesn't exist.

Merges [36094] to the 4.4 branch.
Props tharsheblows.
See #15963.
Fixes #35084.

#24 follow-up: @OSD
9 years ago

Is this problem solved in WordPress 4.4.1 ?

Just asking before updating from 4.3.

#25 in reply to: ↑ 24 @johnbillion
9 years ago

Replying to OSD:

Is this problem solved in WordPress 4.4.1 ?

Yes.

Note: See TracTickets for help on using tickets.