Make WordPress Core

Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#33045 closed enhancement (fixed)

New conditional tags for child/parent pages

Reported by: ramiy's profile ramiy Owned by: johnbillion's profile johnbillion
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch has-unit-tests commit
Focuses: template Cc:

Description

New enhancements for theme developers with new conditional tags:

  • has_parent() - return boolean result using $post->post_parent.
  • parent_page() - return the parent page id using $post->post_parent.
  • is_child_of( $id ) - return boolean result using $post->post_parent.

We can change the names, but we need the functionality.

Attachments (10)

33045.patch (993 bytes) - added by ramiy 9 years ago.
33045.2.patch (967 bytes) - added by ramiy 9 years ago.
33045.3.patch (1.0 KB) - added by sebastian.pisula 9 years ago.
33045.4.patch (848 bytes) - added by ramiy 9 years ago.
33045.5.patch (848 bytes) - added by ramiy 5 years ago.
33045.6.patch (824 bytes) - added by ramiy 5 years ago.
33045.7.diff (950 bytes) - added by audrasjb 4 years ago.
New iteration
33045.8.diff (979 bytes) - added by xkon 4 years ago.
33045.9.diff (2.8 KB) - added by xkon 4 years ago.
33045.diff (2.8 KB) - added by johnbillion 4 years ago.

Download all attachments as: .zip

Change History (66)

#1 @ramiy
9 years ago

See the patch.

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

@ramiy
9 years ago

#2 @ramiy
9 years ago

For consistency, the patch is based on the code of has_excerpt() and get_the_excerpt() functions.

@ramiy
9 years ago

#3 @ramiy
9 years ago

  • Keywords has-patch added

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


9 years ago

#5 follow-up: @johnbillion
9 years ago

  • Component changed from General to Posts, Post Types
  • Keywords reporter-feedback 2nd-opinion added

@ramiy What's the use case for filtering the return value of get_parent()?

Also these functions need post in their name to avoid ambiguity. eg. wp_get_post_parent().

#6 @sebastian.pisula
9 years ago

I added my version of patch

#7 in reply to: ↑ 5 @ramiy
9 years ago

  • Keywords reporter-feedback removed

Replying to johnbillion:

@ramiy What's the use case for filtering the return value of get_parent()?

Also these functions need post in their name to avoid ambiguity. eg. wp_get_post_parent().

You are right, there is no reason to filter the returned value. @sebastian.pisula revised my patch, he removed the filter and added the required post to the function name.

#8 @nir_r
9 years ago

+1 for this feature

#9 follow-up: @ramiy
9 years ago

Actually,

I don't agree with the new function name wp_get_post_parent(),
I prefer it would be get_post_parent(), without the wp_ prefix.

We should stick with the naming conventions in post-template.php, non of the get functions use the wp_ prefix:

  • get_the_ID()
  • get_the_title()
  • get_the_guid()
  • get_the_content()
  • get_the_excerpt()
  • get_post_class()
  • get_body_class()
Last edited 9 years ago by ramiy (previous) (diff)

@ramiy
9 years ago

#10 in reply to: ↑ 9 @maor
9 years ago

Love these functions! Great idea.

I actually just wrapped up a project where these functions could have been super helpful if existed.

Also, I agree with @ramiy on the naming convention. IMHO, I think there shouldn't be a wp_ prefix for the proposed conditional tags.

#11 @ramiy
9 years ago

Same logic can be applied for the boolean function, rename back from has_post_parent() to has_parent().

The other has_() functions don't use the post in their name:

  • has_excerpt()
  • has_tag()
  • has_term()
  • has_nav_menu()

Only one function use post in the name:

  • has_post_thumbnail()

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


9 years ago

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


7 years ago

#14 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 4.9

#15 @ramiy
7 years ago

  • Keywords 2nd-opinion removed

@SergeyBiryukov Let's add this enhancement to 4.9. The ticket has a patch, it was tested and got a very good feedback from @johnbillion and @sebastian.pisula.

The only issue here was the function names. In the last patch I applied the naming conventions set in post-template.php to match similar template functions.

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


7 years ago

#17 @johnbillion
7 years ago

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

#18 @johnbillion
7 years ago

I've decided these functions should be named get_parent_post() and has_parent_post(). The "parent post" is the object being gotten.

This also allows functions such as get_parent_term() to be introduced in the future in order to fetch a term's parent.

#19 @johnbillion
7 years ago

And just as I submitted that I questioned it. Maybe it needs to be get_parent_post_id(). Naming things is hard.

#20 @johnbillion
7 years ago

  • Milestone changed from 4.9 to Future Release

#21 @SergeyBiryukov
7 years ago

For reference, comment:28:ticket:24164 has a good overview of existing get_*() and the_*() template functions.

Until a new function is added, get_post_field( 'post_parent', $post ) could be used for the same purpose.

#22 @desrosj
5 years ago

  • Keywords needs-refresh added

@ramiy
5 years ago

#23 @ramiy
5 years ago

  • Keywords needs-refresh removed

@ramiy
5 years ago

#24 @ramiy
5 years ago

The last patch uses "short if" to match the code to get_the_ID().

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


5 years ago

#26 @birgire
5 years ago

In 33045.6.patch for the has_post_parent() docs, there's currently:

* @return int

but it should be:

 * @return bool

Regarding the naming, I would assume beforehand that get_post_parent() would return a WP_Post object, just as get_post().

But beside from that, we already have this function in core:

/**
 * Returns the ID of the post's parent.
 *
 * @since 3.1.0
 *
 * @param int|WP_Post $post Post ID or post object. Defaults to global $post.
 * @return int|false Post parent ID (which can be 0 if there is no parent), or false if the post does not exist.
 */
function wp_get_post_parent_id( $post ) {
        $post = get_post( $post );
        if ( ! $post || is_wp_error( $post ) ) {
                return false;
        }
        return (int) $post->post_parent;
}

See https://developer.wordpress.org/reference/functions/wp_get_post_parent_id/

Last edited 5 years ago by birgire (previous) (diff)

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


5 years ago

#28 @johnbillion
5 years ago

  • Owner johnbillion deleted

#29 @audrasjb
4 years ago

  • Keywords needs-testing dev-feedback added
  • Milestone changed from Future Release to 5.7
  • Owner set to audrasjb

Moving for 5.7 consideration

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


4 years ago

@audrasjb
4 years ago

New iteration

#31 @audrasjb
4 years ago

  • Keywords 2nd-opinion added; needs-testing dev-feedback removed
  • Status changed from reviewing to accepted

Hi there,

33045.7.diff is a new iteration for this ticket:

  • @since docs update
  • get_post_parent() now returns parent post object
  • few DocBlock enhancements

#32 @audrasjb
4 years ago

  • Keywords needs-dev-note needs-docs added

#33 @audrasjb
4 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

As Core Tech Lead for 5.7, @SergeyBiryukov, what's your feeling about those changes? I think it's pretty neat but feel free to take ownership on the ticket for proper review before the end of the alpha cycle ;)

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


4 years ago

#35 @johnbillion
4 years ago

  • Keywords needs-unit-tests added; dev-feedback removed

These need tests. The functions in the latest patch do not work as expected.

#36 follow-up: @joyously
4 years ago

I don't think the function names should have 'post' in them, because the standard Post is not hierarchical.
These functions don't seem very useful either, as we've gone more than 15 years without them just fine.

#37 in reply to: ↑ 36 @ramiy
4 years ago

Thank you @xkon, love the new checks you added, looks much better.

@xkon
4 years ago

#38 @xkon
4 years ago

Hey @ramiy, thanks but I've re-uploaded 33045.8.diff. It seems that they where not really needed as get_post() takes care of it as it seems on it's own either way, so I reverted to the original :) with the minor fix mentioned below.

@audrasjb I took a look on 33045.7.diff and instead of returning $post->post_parent it was returning $post again in get_post_parent() so it wasn't working as expected.

I'll try to work on unit tests as well to get this ready :). It should be good now though.

@xkon
4 years ago

#39 @xkon
4 years ago

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

33045.9.diff adds unit tests, also renames the functions into has_parent_post() & get_parent_post() as mentioned by @johnbillion in comment 18 as it makes more sense.

I believe this is ready for a review if I didn't miss anything extra from the previous comments.

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


4 years ago

#41 @audrasjb
4 years ago

  • Keywords commit added

Thanks for the new patch and for adding unit tests.
I think we're ready for 5.7 beta 1 👌

@johnbillion
4 years ago

#42 @johnbillion
4 years ago

This looks good. 33045.diff just tweaks the docs and switches to using the post factory in the tests.

One remaining concern that I haven't seen mentioned yet is whether any existing plugins or themes already define either a get_parent_post() or has_parent_post() function. If they do, a fatal error will occur in 5.7.

I checked the plugin and theme directories and (amazingly?) only one plugin is affected,Revisionize with 6,000 active installations, and it defines the get_parent_post() function. No themes are affected.

I think we can reach out to the plugin author and ask them to update the plugin with a function_exists() check. If it's absolutely necessary we can ask the plugin team to push out an update with that change before 5.7.

Unless anybody disagrees, I'll commit this this evening.

#43 @audrasjb
4 years ago

Good point @johnbillion. I just opened a pull request on their GitHub repository and a thread on their support forum :)

#44 @johnbillion
4 years ago

  • Owner changed from audrasjb to johnbillion

#45 @johnbillion
4 years ago

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

In 50127:

Posts, Post Types: Introduce new functions for determining if a post has a parent (has_post_parent()) and to fetch the post parent (get_post_parent()).

These functions are simple but reduce the logic needed in themes and plugins.

Props ramiy, sebastian.pisula, birgire, audrasjb, xkon

Fixes #33045

#46 follow-up: @johnjamesjacoby
4 years ago

Hey @johnbillion!

These functions are named weird. 😕

Your commit message uses post_parent() but the functions that were committed use parent_post().

I’ve read all of the comments in this ticket, and see it’s gone back and forth until you put your foot down 3 years ago, but then your commit message hilariously inverted them, and my intuition tells me that post-parent is more natural than parent-post.

If left as-is, I think it is highly likely that everyone until the end of time is going to get these function names wrong in their code-editor auto-completes.

I.E. I would never think to type get_status_post and I’ll never remember to type get_parent_post.

Thoughts?

#47 @johnbillion
4 years ago

  • Keywords 2nd-opinion added; has-patch needs-dev-note needs-docs has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Naming things is hard.

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


4 years ago

#49 in reply to: ↑ 46 @SergeyBiryukov
4 years ago

Replying to johnjamesjacoby:

I’ve read all of the comments in this ticket, and see it’s gone back and forth until you put your foot down 3 years ago, but then your commit message hilariously inverted them, and my intuition tells me that post-parent is more natural than parent-post.

I also went back and forth on this when reviewing the commit :) I guess get_post_parent(), for consistency with the post_parent field, would indeed be preferable here.

#50 @lukecarbis
4 years ago

This is a really hard call, but I also like the consistency with the post_parent field. I'm on board with get_post_parent() and has_post_parent().

This ticket was mentioned in PR #1017 on WordPress/wordpress-develop by peterwilsoncc.


4 years ago
#51

  • Keywords has-patch has-unit-tests added

#52 @peterwilsoncc
4 years ago

I've set up a pull request renaming these to *_post_parent(). I'm happy with those as names too, click approve/request changes on the PR to express an opinion either way.

#53 @johnbillion
4 years ago

  • Keywords commit added; 2nd-opinion removed
  • Status changed from reopened to reviewing

Checking the plugin and theme directories:

  • get_post_parent for plugins ref
    • Four matches, all four are class methods
  • get_post_parent for themes ref
    • No matches
  • has_post_parent for plugins ref
    • No matches
  • has_post_parent for themes ref
    • No matches

#54 @johnbillion
4 years ago

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

In 50396:

Posts, Post Types: Rename the new post parent conditional tag functions for clarity.

  • get_parent_post() becomes get_post_parent()
  • has_parent_post() becomes has_post_parent()

Props johnjamesjacoby, SergeyBiryukov, lukecarbis, peterwilsoncc

Fixes #33045

Note: See TracTickets for help on using tickets.