Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#57131 new enhancement

get_the_terms(): Parameter #1 ($post) of type is nullable.

Reported by: omaeyusuke's profile omaeyusuke Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.6
Component: Taxonomy Keywords: has-patch 2nd-opinion
Focuses: docs Cc:

Description

Follow-up to [24616].

From documentation on the return value of get_the_terms(), and the usage of get_the_category(), it may be assumed that this function may be used in the following manner.

<?php
$term = get_the_terms( null, 'category' );

Change History (8)

This ticket was mentioned in PR #3639 on WordPress/wordpress-develop by @omaeyusuke.


2 years ago
#1

  • Keywords has-patch added

From documentation on the return value of get_the_terms(), and the usage of get_the_category(), it may be assumed that this function may be used in $term = get_the_terms( null, 'category' );.

Trac ticket: https://core.trac.wordpress.org/ticket/57131

#2 follow-up: @SergeyBiryukov
2 years ago

Hi there, welcome back to WordPress Trac! Thanks for the ticket and the PR.

Looking at the other taxonomy functions, including get_the_category_list(), in_category(), the_category(), has_category(), has_term(), etc., it seems like we don't generally include null as an accepted value for the optional $post or $post_id parameter, but we do note "Defaults to the current post".

For get_the_terms() function it's a bit different because, unlike in the above functions, the $post parameter is required, not optional.

I'm not quite sure about this change. While technically $post can be null here, passing the ID explicitly as the current documentation suggests seems more readable and future-proof:

$term = get_the_terms( get_the_ID(), 'category' );

#3 follow-up: @SergeyBiryukov
2 years ago

  • Keywords 2nd-opinion added

Curious to see what others think.

@audrasjb commented on PR #3639:


2 years ago
#4

Hello and thanks for the patch. Looks like there is an indentation issue in your patch, though :)

#5 in reply to: ↑ 3 @peterwilsoncc
2 years ago

Replying to SergeyBiryukov:

Curious to see what others think.

I think it becomes a question of what WordPress documents: is it the recommended approach or what is technically feasible.

To get the terms for the current global post, I would personally recommend get_the_terms( get_the_ID() /* or get_post() */, 'category' ); to provide clarity; passing null is technically possible and likely will remain supported.

I'm in two minds but am leaning towards documenting the technically possible that null is supported.

If they don't exist already, I'd certainly like to see some tests added to ensure compatibility is maintained.

#6 in reply to: ↑ 2 @omaeyusuke
2 years ago

Replying to SergeyBiryukov:
Thank you fast review!

Looking at the other taxonomy functions, including get_the_category_list(), in_category(), the_category(), has_category(), has_term(), etc., it seems like we don't generally include null as an accepted value for the optional $post or $post_id parameter, but we do note "Defaults to the current post".

Yes! IMO $post is required. but, when WordPress uses get_the_terms(), it is implemented with the assumption that it may send a post ID that does not exist.

I'm not quite sure about this change. While technically $post can be null here, passing the ID explicitly as the current documentation suggests seems more readable and future-proof:

$term = get_the_terms( get_the_ID(), 'category' );

get_the_ID() is false if the post does not exist. Therefore, the following statement is strictly inconsistent from the documentation of get_the_terms().

<?php
$term = get_the_terms( get_the_ID() /* false if non-existent post. */, 'category' );

Also, since you are accepting int, there is no option not to use get_post() inside the function to check for non-existent posts, then I see no thin ...... reason not to accept nullable.

Last edited 2 years ago by omaeyusuke (previous) (diff)

@omaeyusuke commented on PR #3639:


2 years ago
#7

Hello and thanks for the patch. Looks like there is an indentation issue in your patch, though :)

Thank you! I fixed it.

#8 @omaeyusuke
2 years ago

I have rethought my current implementation and understand that get_post() is only used to check for the existence of posts, and that receiving WP_Post is an extra if we are talking about implementation alone.
However, if I make it not-nullable, the current implementation of get_the_category() breaks. There is a way to rewrite it, which is what I'm trying to do.

Version 0, edited 2 years ago by omaeyusuke (next)
Note: See TracTickets for help on using tickets.