#35084 closed defect (bug) (fixed)
check for post status in get_page_uri causes issues with permalinks
Reported by: | tharsheblows | Owned by: | 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)
Change History (30)
This ticket was mentioned in Slack in #bbpress by tharsheblows. View the logs.
9 years ago
#2
@
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:
↓ 4
@
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
@
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
@
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.
#6
@
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.
#8
@
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:
- 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.
#9
follow-up:
↓ 10
@
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:
↓ 12
@
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 putget_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. 4287 4287 * 4288 4288 * @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 4289 4291 * 4290 4292 * @param WP_Post|object|int $page Page object or page ID. 4291 4293 * @return string|false Page URI, false on error. … … 4300 4302 4301 4303 foreach ( $page->ancestors as $parent ) { 4302 4304 $parent = get_post( $parent ); 4303 if ( 'publish' === $parent->post_status ){4305 if ( ! in_array( $parent->post_status, get_post_stati( array( 'internal' => true ) ) ) ){ 4304 4306 $uri = $parent->post_name . '/' . $uri; 4305 4307 } 4306 4308 }
I'm not sure if we do the minor version for our docs, i.e. the 4.4.1
line in this case
#12
in reply to:
↑ 10
;
follow-up:
↓ 13
@
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.
#13
in reply to:
↑ 12
;
follow-up:
↓ 14
@
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
@
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
@
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.
This ticket was mentioned in Slack in #core by netweb. View the logs.
9 years ago
#17
@
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
@
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
#20
@
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.
#22
@
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.
change post status to check that it's not in the trash