check for post status in get_page_uri causes issues with permalinks

[tharsheblows]

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.

[tharsheblows]

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

[swissspidy]

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.

[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?

[swissspidy]

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.

[tharsheblows]

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]

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

[swissspidy]

Sweet, thanks for the unit tests!

Moving to 4.4.1 milestone since this is a regression.

[mynamevenu24]

'internal'=>true and add unit test

[netweb]

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:

• ​http://src.wordpress-develop.dev/forums/forum/hidden-category/hidden-forum/
• ​http://src.wordpress-develop.dev/forums/forum/private-category/private-forum/

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

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

[netweb]

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

@@ -4286 +4286 @@
  * Sub pages will be in the "directory" under the parent page post name.
  *
  * @since 1.5.0
+ * @since 4.4.0 Do not add parent slugs to orphaned pages
+ * @since 4.4.1 Support custom post statuses
  *
  * @param WP_Post|object|int $page Page object or page ID.
  * @return string|false Page URI, false on error.
@@ -4300 +4302 @@
 
         foreach ( $page->ancestors as $parent ) {
                 $parent = get_post( $parent );
-                if ( 'publish' === $parent->post_status ) {
+                if ( ! in_array( $parent->post_status, get_post_stati( array( 'internal' => true ) ) ) ){
                         $uri = $parent->post_name . '/' . $uri;
                 }
         }

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

[DrewAPicture]

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]

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)

[DrewAPicture]

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" :-)

[tharsheblows]

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.

[dd32]

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.

[tharsheblows]

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]

only ignore parent in permalink if it is not in database

[tharsheblows]

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

[netweb]

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

[jorbin]

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

[dd32]

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.

[dd32]

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

[dd32]

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.

[OSD]

Is this problem solved in WordPress 4.4.1 ?

Just asking before updating from 4.3.

[johnbillion]

Replying to OSD:

Is this problem solved in WordPress 4.4.1 ?

Yes.