WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#48166 closed defect (bug) (invalid)

Cannot create a new CPT when passing NULL to taxonomies

Reported by: scarr Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.2.3
Component: Posts, Post Types Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

When calling register_post_type() where $args does NOT include taxononies details ('taxonomies = NULL), the following message is displayed:

Warning: Invalid argument supplied for foreach() in ..\wp-includes\class-wp-post-type.php on line 597

This is because register_post_type contains a call to $post_type_object->register_taxonomies().

Bug in wp-includes/class-wp-posst-type.php function = register_taxonomies()

Current:

	public function register_taxonomies() {
		foreach ( $this->taxonomies as $taxonomy ) {
			register_taxonomy_for_object_type( $taxonomy, $this->name );
		}
	}

Proposed Fix:

	public function register_taxonomies() {
                if (!is_array($this->taxonomies)) { return; }
		foreach ( $this->taxonomies as $taxonomy ) {
			register_taxonomy_for_object_type( $taxonomy, $this->name );
		}
	}

Attachments (1)

48166.patch (500 bytes) - added by dkarfa 6 months ago.

Download all attachments as: .zip

Change History (10)

@dkarfa
6 months ago

#1 @SergeyBiryukov
6 months ago

  • Component changed from General to Posts, Post Types
  • Description modified (diff)

#2 follow-up: @subrataemfluence
6 months ago

  • Keywords 2nd-opinion added

@scarr thanks for the ticket and the proposal, which works well if $taxonomy => null.
The workaround looks good, but I have some thoughts about whether we should go for it or not.

We only use $taxonomies keys when we need to add one or more taxonomies to a custom post type. I don't see a reason to add this key with a NULL value intentionally while registering a CPT.

And if we need to add this check for $taxonomies, it has to be added for $capabilities key too.

What I think is we use these keys to form the associative array ($args), only when we know what value to pass. For passing just NULL, we don't use them.

However, from coding-best-practice perspective, I would say it is a good idea to bring it in, but if we do, we have to incorporate at in other areas of the core as well, like once I mentioned above.

Regarding summary:
A CPT can always be created if no $taxonomies key is used or it is assigned with an empty array like $taxonomies => array(). The only situation it throws the said error when the value is NULL, i.e. $taxonomies => NULL. So, for me the initial summary by @scarr was correct, i.e. Cannot create new CPT when passing NULL to taxonomies.

#3 in reply to: ↑ 2 ; follow-up: @scarr
6 months ago

  • Summary changed from Cannot create a new CPT without taxonomies to Cannot create a new CPT when passing NULL to taxonomies

@subrataemfluence - Thank you for taking swift action on my ticket.

Your characterization of the issue I raised is a much more accurate (and better) re-statement, Cannot create new CPT when passing NULL to taxonomies. I've adjusted the description to reflect it. Further, I agree that $capabilities suffers the same issue.

I also accept your workaround - use "'taxonomies' => array()" instead of "'taxonomies' => NULL" when creating a CPT without taxonomies.

Finally, I will leave it to the Core Team to decide whether to accept my proposal on a "best practices" basis.

@scarr

Replying to subrataemfluence:

@scarr thanks for the ticket and the proposal, which works well if $taxonomy => null.
The workaround looks good, but I have some thoughts about whether we should go for it or not.

We only use $taxonomies keys when we need to add one or more taxonomies to a custom post type. I don't see a reason to add this key with a NULL value intentionally while registering a CPT.

And if we need to add this check for $taxonomies, it has to be added for $capabilities key too.

What I think is we use these keys to form the associative array ($args), only when we know what value to pass. For passing just NULL, we don't use them.

However, from coding-best-practice perspective, I would say it is a good idea to bring it in, but if we do, we have to incorporate at in other areas of the core as well, like once I mentioned above.

Regarding summary:
A CPT can always be created if no $taxonomies key is used or it is assigned with an empty array like $taxonomies => array(). The only situation it throws the said error when the value is NULL, i.e. $taxonomies => NULL. So, for me the initial summary by @scarr was correct, i.e. Cannot create new CPT when passing NULL to taxonomies.

Last edited 6 months ago by scarr (previous) (diff)

#4 in reply to: ↑ 3 @subrataemfluence
6 months ago

Thank you @scarr.

Jut to add, if you don't want to associate any taxonomy with your Custom Post Type, just don't define that key at all. No need to add $taxonomies => array(). It is not mandatory. A CPT is good enough without any taxonomy associated with it.

If $taxonomies key is not defined, the post type will be created without any associated taxonomy. Use this only if you need to add any. For example:

$taxonomies => array( 'category', 'some-other-taxonomy', ... ),

Replying to scarr:

@subrataemfluence - Thank you for taking swift action on my ticket.

Your characterization of the issue I raised is a much more accurate (and better) re-statement, Cannot create new CPT when passing NULL to taxonomies. I've adjusted the description to reflect it. Further, I agree that $capabilities suffers the same issue.

I also accept your workaround - use "'taxonomies' => array()" instead of "'taxonomies' => NULL" when creating a CPT without taxonomies.

Finally, I will leave it to the Core Team to decide whether to accept my proposal on a best "best practices" basis.

@scarr

Replying to subrataemfluence:

@scarr thanks for the ticket and the proposal, which works well if $taxonomy => null.
The workaround looks good, but I have some thoughts about whether we should go for it or not.

We only use $taxonomies keys when we need to add one or more taxonomies to a custom post type. I don't see a reason to add this key with a NULL value intentionally while registering a CPT.

And if we need to add this check for $taxonomies, it has to be added for $capabilities key too.

What I think is we use these keys to form the associative array ($args), only when we know what value to pass. For passing just NULL, we don't use them.

However, from coding-best-practice perspective, I would say it is a good idea to bring it in, but if we do, we have to incorporate at in other areas of the core as well, like once I mentioned above.

Regarding summary:
A CPT can always be created if no $taxonomies key is used or it is assigned with an empty array like $taxonomies => array(). The only situation it throws the said error when the value is NULL, i.e. $taxonomies => NULL. So, for me the initial summary by @scarr was correct, i.e. Cannot create new CPT when passing NULL to taxonomies.

#5 follow-up: @jrf
6 months ago

My two pennies: this sounds more like a case of "doing it wrong" than as a bug to be fixed.

#6 @johnbillion
6 months ago

  • Keywords 2nd-opinion removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Agreed. The $taxonomies argument for register_post_type() is documented as an array. Passing anything else will result in unexpected behaviour.

#7 in reply to: ↑ 5 ; follow-ups: @scarr
6 months ago

Hey, thanks for the encouraging feedback. I'll be sure to jump in to help again!

Perhaps, if the code was a bit better from a best practices perspective, you wouldn't have your time wasted by people like me who "should just read the documentation."

Replying to jrf:

My two pennies: this sounds more like a case of "doing it wrong" than as a bug to be fixed.

#8 in reply to: ↑ 7 @scarr
6 months ago

@jrf - Sorry for the earlier rant. But, I feel compelled to raise this issue:

Please, don’t needlessly force coding restrictions on your customers.

If I don’t want to declare taxonomies inside a cpt declaration, why should I have to memorize the fact that the parameter still needs to be set? Doesn’t it make equal sense to have your customer (the developer) simply ignore setting the parameter all together in that case? You know, make it optional, like you do so many other parameters in WordPress.

And, it does not cost anything from a performance perspective. You are adding an if-statement to your code, thus eliminating the if-statement in my code.

Your code adds:

* to register_taxonomies() in wp-includes/class-wp-post-type.php
if (!is_array($this->taxonomies)) { return; }

My code loses:

* Because I cannot ASSUME good behavior from my customers
If (!is_array($taxomonies) { $taxomonies=array(); }

Your “public function”, register_taxonomies, is now bullet proof and I (the customer) have one less aspect to memorize regarding the WordPress interface. It’s a win-win proposition.

Replying to scarr:

Hey, thanks for the encouraging feedback. I'll be sure to jump in to help again!

Perhaps, if the code was a bit better from a best practices perspective, you wouldn't have your time wasted by people like me who "should just read the documentation."

Replying to jrf:

My two pennies: this sounds more like a case of "doing it wrong" than as a bug to be fixed.

#9 in reply to: ↑ 7 @jrf
6 months ago

Replying to scarr:

Hey, thanks for the encouraging feedback. I'll be sure to jump in to help again!

Perhaps, if the code was a bit better from a best practices perspective, you wouldn't have your time wasted by people like me who "should just read the documentation."

Sorry you feel that way.

Just to clarify though: Trac is for bugs and feature requests, not for user (as you state: developers for WP are also users) support questions.

Your point is a perfectly valid support question for the forums, but it's not a valid bug report.

So based on where you post, you will get a different response.

Your code...

Let me stop you right here: this is not "my" code. This is the WP Core code which is created and maintained by hundreds of people.

The code has historically evolved and carries part of that history with it.

Please do not attack individual developers over the state of the WP code base as it is offensive when you do.

P.S.: believe me, if it was my code, it would look very different...

If I don’t want to declare taxonomies inside a cpt declaration, why should I have to memorize the fact that the parameter still needs to be set?

You don't.

You don't need to remember it and it doesn't need to be set.

The $args parameter in register_post_type() is optional and every single array index within this optional array is, again, optional.

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

Doesn’t it make equal sense to have your customer (the developer) simply ignore setting the parameter all together in that case? You know, make it optional, like you do so many other parameters in WordPress.

It already is and will default to an empty array.

See: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-post-type.php#L409

Your “public function”, register_taxonomies, ...

I'd guess that the only reason that function is public is because it came from PHP4 which didn't have visibility indicators, though I have to admit I've not traced back the history of the class.
If my guess is right, changing the visibility on previously published classes like that is a backward-compatibility break which is something WP strenuously tries to avoid...

Note: See TracTickets for help on using tickets.