WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 2 months ago

#50653 new defect (bug)

Remove the _doing_it_wrong from WP_Block_Patterns_Registry::unregister()

Reported by: johnbillion Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.5
Component: Editor Keywords: 2nd-opinion needs-patch
Focuses: Cc:

Description

There's a _doing_it_wrong() call inside WP_Block_Patterns_Registry::unregister() when you try to unregister a block pattern that doesn't exist.

IMO this is incorrect usage of _doing_it_wrong() because the function hasn't been incorrectly called, it's just been called with invalid data.


In addition, the register() and unregister() functions in this class ought to be returning a WP_Error instead of boolean false. Should we improve this for 5.5?

Change History (10)

#1 follow-up: @youknowriad
3 months ago

I don't feel strongly personally but this is modeled over existing block style variations and block types registries. If we make a change, we should check that it's consistent with other registries.

This ticket was mentioned in Slack in #core by noisysocks. View the logs.


3 months ago

#3 @isabel_brison
3 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.5

#4 @isabel_brison
3 months ago

Let's remove the _doing_it_wrong() in the Block Patterns Registry for 5.5 as that registry is only just being added, and then remove any other incorrectly used instances later for consistency.

This ticket was mentioned in PR #404 on WordPress/wordpress-develop by donmhico.


3 months ago

  • Keywords has-patch added; needs-patch removed

Remove _doing_it_wrong() in WP_Block_Patterns_Registry::register() and WP_Block_Patterns_Registry::unregister() and return WP_Error instead of false when used with incorrect data.

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

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


2 months ago

#7 @desrosj
2 months ago

At first I thought since this would change the data type that is returned, it could be a risky change. However, block patterns have not shipped in core yet, so I think this is a fine change to make. Only plugins that are building off the Gutenberg plugin would potentially have an issue. But the chance they are calling unregister() incorrectly is probably slim.

#8 in reply to: ↑ 1 @SergeyBiryukov
2 months ago

  • Keywords 2nd-opinion added

Replying to youknowriad:

I don't feel strongly personally but this is modeled over existing block style variations and block types registries. If we make a change, we should check that it's consistent with other registries.

I agree with that. For reference, here's the list of classes and methods that follow the pattern:

  • WP_Block_Pattern_Categories_Registry::(un)register() (introduced in 5.5.0).
  • WP_Block_Patterns_Registry::(un)register() (introduced in 5.5.0)
  • WP_Block_Styles_Registry::(un)register() (introduced in 5.3.0)
  • WP_Block_Type_Registry::(un)register() (introduced in 5.0.0)

While there should be no back compat concerns in changing the former two, there should be an investigation of possible side effects for changing the latter two.

While I agree switching all of these to return WP_Error on failure would make sense, changing only one or two classes would bring an inconsistency without a strong reason. I think being consistent in our APIs outweights the benefits of such a change.

I'd suggest either changing all of these at once, or just going with the existing approach.

#9 @whyisjake
2 months ago

@donmhico can you update the PR to account for the four cases above?

Last edited 2 months ago by whyisjake (previous) (diff)

#10 @SergeyBiryukov
2 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 5.5 to Future Release
Note: See TracTickets for help on using tickets.