WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 17 months ago

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

Download all attachments as: .zip

Change History (31)

@spacedmonkey
2 years ago

#1 @spacedmonkey
2 years ago

This ticket is a blocker for #47620.

#2 @gziolo
2 years ago

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

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

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

Good point @aduth.

I can remove that check.

#8 @spacedmonkey
17 months ago

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

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

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

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

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

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

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Correct one

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

In 47908:

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

Follow-up to [47907].

See #48529.

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