WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 5 months ago

Last modified 4 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 12 months ago.
48529-implement.diff (8.8 KB) - added by spacedmonkey 12 months ago.
48529.2.diff (5.1 KB) - added by spacedmonkey 6 months ago.
48529.3.diff (5.8 KB) - added by gziolo 5 months ago.
48529.4.diff (1.8 KB) - added by gziolo 5 months ago.
Correct one

Download all attachments as: .zip

Change History (31)

@spacedmonkey
12 months ago

#1 @spacedmonkey
12 months ago

This ticket is a blocker for #47620.

#2 @gziolo
12 months ago

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

#3 @spacedmonkey
12 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 12 months ago by spacedmonkey (previous) (diff)

#4 follow-up: @aduth
12 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
12 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
12 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
12 months ago

Good point @aduth.

I can remove that check.

#8 @spacedmonkey
6 months ago

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

#9 follow-up: @gziolo
6 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
6 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
6 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
6 months ago

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

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

#13 @gziolo
5 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
5 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
5 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
5 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
5 months ago

#17 @gziolo
5 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 5 months ago by gziolo (previous) (diff)

#18 @gziolo
5 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
5 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
5 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
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#22 @gziolo
5 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
5 months ago

Correct one

#23 @gziolo
5 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
5 months ago

In 47908:

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

Follow-up to [47907].

See #48529.

#25 @gziolo
4 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
4 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.