Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#48401 closed defect (bug) (fixed)

Saving a Post Breaks in Gutenberg if you have a custom taxonomy named Status

Reported by: kingkool68's profile kingkool68 Owned by: kadamwhite's profile kadamwhite
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: good-first-bug has-unit-tests has-patch commit
Focuses: Cc:


Steps to reproduce:

Register a taxonomy called Status:

function custom_taxonomy() {

	$labels = array(
		'name'                       => 'Status',
	$args = array(
		'labels'                     => $labels,
		'hierarchical'               => false,
		'public'                     => true,
		'show_ui'                    => true,
		'show_admin_column'          => true,
		'show_in_nav_menus'          => true,
		'show_tagcloud'              => true,
		'show_in_rest'               => true,
	register_taxonomy( 'status', array( 'post' ), $args );

add_action( 'init', 'custom_taxonomy', 0 );

Create a new Post using the Gutenberg editor
Try and Publish the post
Publishing should fail.
Add rest_base => 'status_tax' and publishing a post should work as expected.

The issue is if the rest_base argument isn't specified register_taxonomy() will use the taxonomy key as the rest base. In this case that would be status which is already a REST API route.

There should be a check to make sure a reserved REST API base isn't used. Ideally it would modify the default used to avoid the conflict such as appending _tax. Or atleast an error or warning should be thrown somewhere.

There is some confusion out there that this is a conflict between different plugins as documented here -->

Attachments (3)

48401.diff (2.0 KB) - added by fgiannar 4 years ago.
48401.2.diff (2.3 KB) - added by fgiannar 4 years ago.
48401.3.diff (2.5 KB) - added by TimothyBlynJacobs 4 years ago.

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
5 years ago

Thanks for the ticket!

Apparently status should be added to the list of reserved terms (I thought it's already there).

A _doing_it_wrong() notice could also be added to avoid the conflict.

#2 @desrosj
4 years ago

  • Keywords needs-patch added

#3 @TimothyBlynJacobs
4 years ago

  • Keywords good-first-bug needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

I agree a _doing_it_wrong would be a good addition. Perhaps in get_item_schem issue a warning if a property is already set with the same name on the schema?

4 years ago

#4 @fgiannar
4 years ago

  • Keywords needs-patch needs-unit-tests removed

Hi @TimothyBlynJacobs ,

I have added a patch that issues a warning if a property is already set with the same name on the schema, as you mention.
I also included the corresponding unit test.

Please make sure to double check the version in _doing_it_wrong as I was uncertain what the correct value should be.

Also, I was wondering whether the same warning that we issue to get_item_schema for taxonomies should be also issued in add_additional_fields_schema.

Finally, I was unsure whether I should unregister the taxonomy I use in the unit test.

This is one of my first tickets so your feedback would be more than welcome.


#5 @fgiannar
4 years ago

  • Keywords has-patch added

#6 @TimothyBlynJacobs
4 years ago

  • Keywords needs-refresh has-unit-tests added; has-patch removed
  • Milestone changed from Future Release to 5.4
  • Owner set to fgiannar
  • Status changed from new to assigned

Thanks for working on this @fgiannar, this looks great! That version number is correct, and the tests look good.

Instead of setting the function on _doing_it_wrong to the current method, I think it might make more sense for the function to be register_taxonomy as that is where the developer would have to make their adjustments. I think it would also be helpful to add a bit more context to the message. Something along the lines of...

The %s taxonomy name conflicts with an existing property on the REST API Posts Controller. Specify a custom "rest_base" when registering the taxonomy to avoid this error.

#7 @fgiannar
4 years ago

Thanks a lot for your feedback @TimothyBlynJacobs !

Just a question please on your suggested message:
I think that the _doing_it_wrong warning could be issued not only if the taxonomy name conflicts with an existing property, but also the rest_base, as the code to decide on the base is:
$base = ! empty( $taxonomy->rest_base ) ? $taxonomy->rest_base : $taxonomy->name;

So, if for example, one had set 'status' as the rest_base the same warning would be produced.
Maybe if we only mention the name on the warning it could be misleading.

What do you think?

#8 @TimothyBlynJacobs
4 years ago

That's a great point! I think including the rest base would be a good idea.

4 years ago

#9 @fgiannar
4 years ago

@TimothyBlynJacobs I have added a new patch with the discussed updates.
While testing this I also noticed that it is possible to register two different taxonomies with the same rest_base.

register_taxonomy( 'tax_1', array( 'post' ), array( 'show_in_rest' => true, 'rest_base' => 'common' ) );
register_taxonomy( 'tax_2', array( 'post' ), array( 'show_in_rest' => true, 'rest_base' => 'common' ) );

This also causes the warning to be displayed, but this time because the common custom taxonomy rest_base of tax_2 conflicts with the already registered property of the tax_1 taxonomy rest_base.
I think it's alright to also show the warning in this case.
Do you believe it would also make sense to open another ticket for preventing registering two taxonomies with the same custom rest_base or the current one should be considered enough?

#10 @TimothyBlynJacobs
4 years ago

Thanks @fgiannar!

Interesting, I think we can keep showing the warning in the same place we are now. Could we easily detect that the conflict is coming from another taxonomy and include that taxonomy in the message if that is the case?

#11 @fgiannar
4 years ago

I guess we could keep an array of the taxonomy bases and check against it or keep a copy of the schema properties before starting the taxonomy loop, but I wouldn't like the idea of adding extra code outside the conditional check that produces the warning only for utilising it there...

Technically the message is correct, since if the error is produced because of 2 taxonomies sharing a common rest_base, the first one is already defined as an existing schema property.

Nevertheless, if you believe it's best to differentiate the warnings let me know and I will submit another patch.

#12 @TimothyBlynJacobs
4 years ago

Ok, let's stick with the single warning message for now!

#13 @fgiannar
4 years ago

@TimothyBlynJacobs Sure, please let me know if I need to do anything in addition, eg update ticket details etc (I'm new to this, so a bit unsure what the proper process is).
Thanks a lot for your time and guidance!

#14 @TimothyBlynJacobs
4 years ago

  • Keywords has-patch added; needs-refresh removed
  • Version set to 4.7

No problem! At this point, nothing further is needed. Another committer will review the patch, and assuming all is good we'll get it committed.

#15 @TimothyBlynJacobs
4 years ago

Uploaded a revised version of the patch to add a translators comment to the warning message. Translator comments are always required when using sprintf placeholders within a translatable string.

#16 @kadamwhite
4 years ago

  • Keywords commit added
  • Owner changed from fgiannar to kadamwhite

#17 @kadamwhite
4 years ago

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

In 47037:

REST API: Issue doing_it_wrong if a taxonomy's specified rest_base is already in use by a different resource.

Props fgiannar, TimothyBlynJacobs, kingkool68, SergeyBiryukov.
Fixes #48401.

#18 @SergeyBiryukov
4 years ago

In 47045:

REST API: Display the taxonomy name in the _doing_it_wrong() message for a conflicting rest_base or name property, to match the translator comment.

Follow-up to [47037].

See #48401.

#19 @SergeyBiryukov
4 years ago

In 47046:

REST API: Display the actual conflicting value in addition to the taxonomy name in the _doing_it_wrong() message for a conflicting rest_base or name property.

Follow-up to [47037] and [47045].

Props TimothyBlynJacobs.
See #48401.

Note: See TracTickets for help on using tickets.