WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#48401 closed defect (bug) (fixed)

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

Reported by: kingkool68 Owned by: 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:
PR Number:

Description

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 --> https://wordpress.org/support/topic/conflict-between-cpt-ui-and-acf-pro-in-the-block-editor/

Attachments (3)

48401.diff (2.0 KB) - added by fgiannar 6 weeks ago.
48401.2.diff (2.3 KB) - added by fgiannar 5 weeks ago.
48401.3.diff (2.5 KB) - added by TimothyBlynJacobs 3 weeks ago.

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
3 months 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
3 months ago

  • Keywords needs-patch added

#3 @TimothyBlynJacobs
2 months 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?

@fgiannar
6 weeks ago

#4 @fgiannar
6 weeks 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.

Thanks,

#5 @fgiannar
6 weeks ago

  • Keywords has-patch added

#6 @TimothyBlynJacobs
6 weeks 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
5 weeks 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
5 weeks ago

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

@fgiannar
5 weeks ago

#9 @fgiannar
5 weeks 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.
eg

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
5 weeks 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
5 weeks ago

@TimothyBlynJacobs
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
5 weeks ago

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

#13 @fgiannar
5 weeks 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
5 weeks 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
3 weeks 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
3 weeks ago

  • Keywords commit added
  • Owner changed from fgiannar to kadamwhite

#17 @kadamwhite
3 weeks 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
3 weeks 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
3 weeks 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.