WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 14 months ago

Last modified 13 months ago

#48529 closed enhancement (fixed)

Add fields to WP_Block_Type

Reported by: spacedmonkey Owned by: spacedmonkey
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.0
Component: Editor Keywords: has-patch has-dev-note has-unit-tests commit
Focuses: Cc:

Description

As part of #47620 and the RFC for block registeration. Server registered blocks are missing some fields. These fields include.

  • title
  • category
  • parent
  • icon
  • description
  • keywords
  • textDomain
  • styleVariations

Recommended addictional fields

  • supports
  • example

Attachments (5)

48529.diff (3.7 KB) - added by spacedmonkey 21 months ago.
48529-implement.diff (8.8 KB) - added by spacedmonkey 21 months ago.
48529.2.diff (5.1 KB) - added by spacedmonkey 15 months ago.
48529.3.diff (5.8 KB) - added by gziolo 14 months ago.
48529.4.diff (1.8 KB) - added by gziolo 14 months ago.
Correct one

Download all attachments as: .zip

Change History (31)

@spacedmonkey
21 months ago

#1 @spacedmonkey
21 months ago

This ticket is a blocker for #47620.

#2 @gziolo
21 months ago

This looks great, what's necessary to test it and land it? Does it require changes in unit tests?

#3 @spacedmonkey
21 months ago

Good question @gziolo . I will take a look. But the tests should be simple.

We also need to add the fields in for currently registered blocks. I made a start with 48529-implement.diff.

We will also need to update the social links in the plugin.

Last edited 21 months ago by spacedmonkey (previous) (diff)

#4 follow-up: @aduth
21 months ago

There's at least one condition which tests isset on a property of this class, where we're now always assigning a default. Do those need to be updated?

https://github.com/WordPress/wordpress-develop/blob/6cf77db/src/wp-includes/class-wp-block-type.php#L134-L138

#5 in reply to: ↑ 4 @spacedmonkey
21 months ago

Replying to aduth:

There's at least one condition which tests isset on a property of this class, where we're now always assigning a default. Do those need to be updated?

What if someone doesn't register attributes in their plugin for their custom block. That check might be overkill, but might stop a breakable if someone does something silly.

#6 @aduth
21 months ago

But unless I'm mistaken, since the patch proposes to assign a default value for $attributes, that condition could never be true (unless someone went out of their way to unset( $block_type->attributes )).

#7 follow-up: @spacedmonkey
21 months ago

Good point @aduth.

I can remove that check.

#8 @spacedmonkey
15 months ago

@aduth @gziolo Any chance of getting this in 5.5?

#9 follow-up: @gziolo
15 months ago

https://core.trac.wordpress.org/attachment/ticket/48529/48529.diff looks good, one note:

 'title'           => $this->name,

Name and title are 2 different things.

https://core.trac.wordpress.org/attachment/ticket/48529/48529-implement.diff - this should be done on the Gutenberg side because those files are copied over from node_modules folder.

#10 in reply to: ↑ 9 @spacedmonkey
15 months ago

  • Keywords has-dev-note needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.5

https://core.trac.wordpress.org/attachment/ticket/48529/48529-implement.diff - this should be done on the Gutenberg side because those files are copied over from node_modules folder.

Never really thought this would go in the commit, which is it why it is a different patch file.

This needs to tests.

#11 @gziolo
15 months ago

Two more things:

Parent set as an empty array prevents the block from appearing in the inserter, it should remain undefined:
https://developer.wordpress.org/block-editor/developers/block-api/block-registration/#parent-optional

Example is an object in JavaScript, so here it probably should be an array rather than boolean, it should be undefined by default:
https://developer.wordpress.org/block-editor/developers/block-api/block-registration/#example-optional

#12 @spacedmonkey
15 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Updated the patch for 5.5. Can you review again @gziolo ?

#13 @gziolo
15 months ago

  • Keywords commit added

Nice, it looks great, let's get it in.

One non-blocking nitpick regarding the test:

'text_domain' => [ 'test_domain' ],

It should be a string rather than an array.

#14 in reply to: ↑ 7 @aduth
15 months ago

Replying to spacedmonkey:

Good point @aduth.

I can remove that check.

Was this ever addressed? I don't see a change in the latest patch.

#15 @gziolo
14 months ago

One more thing, it looks like textdomain is the preferred way of spelling it:
https://github.com/WordPress/wordpress-develop/search?q=textdomain&unscoped_q=textdomain

Should we follow it?

#16 @gziolo
14 months ago

Oh, and we should use styles without the alias for styleVariations as discussed in https://github.com/WordPress/gutenberg/pull/22519#discussion_r430981909

@gziolo
14 months ago

#17 @gziolo
14 months ago

I applied a few changes with the latest patch version uploaded:

  • renamed text_domain to textdomain (seems to be preferred in WordPress core, https://github.com/WordPress/wordpress-develop/search?q=textdomain&unscoped_q=textdomain) and changed the default value to null to reflect the fact that it can be ommited
  • removed all references to styleVariations to continue use styles that existed before
  • updated the default value for attributes to null to make it work with the logic pointed by @aduth – we can change it later but I would prefer to keep it out of this ticket because it's very sensitive change
  • updated parent to allow null value and make it the default - it makes it possible to distinguish between [] and null that can have different meanings as pointed in https://github.com/WordPress/gutenberg/pull/22695#issuecomment-635329233 by @aduth
  • example is also sensible to null vs [] logic
Last edited 14 months ago by gziolo (previous) (diff)

#18 @gziolo
14 months ago

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

In 47875:

Add fields to WP_Block_Type

As part of #47620 and the RFC for block registeration. Server registered blocks are missing some fields. These changeset includes them.

Props spacedmonkey, aduth.

Fixes #48529.

#19 @ocean90
14 months ago

In 47876:

Editor: Fix code style for constructor arguments added in [47875].

Also revert unintended changes to WP_Block_Type::__construct() DocBlock.

See #48529.

#20 @aduth
14 months ago

There is some conflict with how the block registration considers these new default values. Debugging details are summarized at the GitHub issue:

https://github.com/WordPress/gutenberg/issues/22810

#21 @gziolo
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#22 @gziolo
14 months ago

@aduth, I included a new diff that in combination with https://github.com/WordPress/gutenberg/pull/22849 should resolve the issues you raised.

@gziolo
14 months ago

Correct one

#23 @gziolo
14 months ago

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

In 47907:

Blocks: Update default value for some fields in WP_Block_Type

Related to the issue with default values for the blocks registered on the server. By using null for some fields we can treat them as undefined on the client.

See: WordPress/gutenberg#22849.
Props aduth.
Fixes #48529.

#24 @SergeyBiryukov
14 months ago

In 47908:

Docs: Update type for WP_Block_Type::$style property.

Follow-up to [47907].

See #48529.

#25 @gziolo
14 months ago

In [48117]:

Blocks: Add context fields to WP_Block_Type

New block context related fields were added as part of https://github.com/WordPress/gutenberg/pull/22686. This changest backports them to WP_Block_Type class.

#26 @desrosj
13 months ago

@spacedmonkey can you clarify if you meant to tag needs-dev-note? There does not seem to be one published yet.

Note: See TracTickets for help on using tickets.