Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add conditional return types for several functions #195

Merged
merged 15 commits into from
Sep 8, 2024

Conversation

IanDelMar
Copy link
Contributor

@IanDelMar IanDelMar commented Aug 23, 2024

This pull request moves the following extensions from szepeviktor/phpstan-wordpress to php-stubs/wordpress-stubs:

  • get_posts()
  • get_approved_comments()
  • get_sites()
  • get_terms()
  • get_tags()
  • wp_get_object_terms()
  • wp_get_post_categories()
  • wp_get_post_tags()
  • wp_get_post_terms()

Note on wp_get_object_terms()

Despite what is suggested in the return type description ("Array of terms, a count thereof as a numeric string, or [...]") and the parameter description for $args ("See WP_Term_Query::__construct() for supported arguments"), it appears that wp_get_object_terms() does not support $args = ['fields' => 'count']. The output from get_terms(), which is a numeric string when ['fields' => 'count'] is used, is combined/merged with an array (see src/wp-includes/taxonomy.php#L2319-L2323). This causes the function to return null and trigger a warning in PHP 7.4, and is leading to a fatal error (uncaught type error) in PHP 8.0 and later.

As wp_get_post_categories(), wp_get_post_tags(), and wp_get_post_terms() are wrapper functions for wp_get_object_terms(), this limitation applies to them as well. While the documented return types for wp_get_post_tags(), wp_get_post_terms(), and wp_get_post_categories() at least imply that they are not meant to return a numeric-string (though possibly a numeric-string[]), the parameter description for $args still incorrectly states that "See WP_Term_Query::__construct() for supported arguments" is applicable.

See core ticket: https://core.trac.wordpress.org/ticket/61936

@swissspidy, @johnbillion,
I would appreciate it if you could review what I've found regarding wp_get_object_terms() and the related functions. I suspect that simply casting the output of get_terms() to an array in wp_get_object_terms() might allow $args = ['fields' => 'count'] to be supported with a return type of list<numeric-string>. I have not tested this, and I am uncertain if such a change would introduce any undesired side effects. However, this has to be fixed in WP core.
Additionally, there is a $count query variable (distinct from the $fields value) documented for WP_Term_Query and referenced in the return type description for get_terms(). I couldn't determine its purpose. Could it be that this variable was mistakenly left after the corresponding functionality was removed? If so, this would also need to be fixed in WP core.

@swissspidy
Copy link
Contributor

No idea about wp_get_object_terms() off the top of my head, haven't really touched taxonomies much lately. Might be easiest to just open a Trac ticket.

Additionally, there is a $count query variable (distinct from the $fields value) documented for WP_Term_Query and referenced in the return type description for get_terms(). I couldn't determine its purpose. Could it be that this variable was mistakenly left after the corresponding functionality was removed? If so, this would also need to be fixed in WP core.

This refers to WP_Term_Query( [ 'count' => true ] ) which is equivalent to WP_Term_Query( [ 'fields' => 'count' ] ). Seems pretty clearly documented in the source code IMO. Doesn't it work for you?

@IanDelMar
Copy link
Contributor Author

Might be easiest to just open a Trac ticket.

That's the part I find a bit challenging. 😅

This refers to WP_Term_Query( [ 'count' => true ] ) which is equivalent to WP_Term_Query( [ 'fields' => 'count' ] ). Seems pretty clearly documented in the source code IMO. Doesn't it work for you?

Yes, it's documented, but it does not work because there is no actual code implementing ['count' => true]. Meanwhile I have learned that I’m not the only one wondering about this: https://core.trac.wordpress.org/ticket/61094

@szepeviktor
Copy link
Member

I would appreciate it if you could review

@IanDelMar
Copy link
Contributor Author

@szepeviktor Should I split the pull request into several smaller ones - such as syncing with master, get_posts, get_approved_comments, get_sites, get_terms, get_tags, and the remaining functions - to make it easier to review?

@szepeviktor
Copy link
Member

No.
Just tell me when to push "Merge".

@swissspidy
Copy link
Contributor

Core ticket for wp_get_object_terms(): https://core.trac.wordpress.org/ticket/61936

@IanDelMar
Copy link
Contributor Author

@szepeviktor I have added the count fields to the conditional return type, as well as checks for the first parameter and, if applicable, for the second parameter in wp_get_object_terms, wp_get_post_terms, wp_get_post_tags, and wp_get_post_categories. I expect the core ticket will take some time to be resolved. I suggest merging these changes, as they reflect the correct behaviour of these functions, which I believe will be the outcome of the core ticket. We could add a rule for the count fields in phpstan-wordpress. As the count fields do not seem to have caused major issues, I don't think it's necessary.

@szepeviktor szepeviktor merged commit d2a203c into php-stubs:master Sep 8, 2024
5 checks passed
@IanDelMar IanDelMar deleted the import-extensions branch September 9, 2024 20:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants