Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#48529 closed enhancement (fixed)

Add fields to WP_Block_Type

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile 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 4 years ago.
48529-implement.diff (8.8 KB) - added by spacedmonkey 4 years ago.
48529.2.diff (5.1 KB) - added by spacedmonkey 4 years ago.
48529.3.diff (5.8 KB) - added by gziolo 4 years ago.
48529.4.diff (1.8 KB) - added by gziolo 4 years ago.
Correct one

Download all attachments as: .zip

Change History (31)

@spacedmonkey
4 years ago

#1 @spacedmonkey
4 years ago

This ticket is a blocker for #47620.

#2 @gziolo
4 years ago

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

#3 @spacedmonkey
4 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.

Version 0, edited 4 years ago by spacedmonkey (next)

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

Good point @aduth.

I can remove that check.

#8 @spacedmonkey
4 years ago

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

#9 follow-up: @gziolo
4 years 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
4 years 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
4 years 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

@spacedmonkey
4 years ago

#12 @spacedmonkey
4 years ago

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

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

#13 @gziolo
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

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

#18 @gziolo
4 years 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
4 years 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
4 years 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
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#22 @gziolo
4 years 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
4 years ago

Correct one

#23 @gziolo
4 years 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
4 years ago

In 47908:

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

Follow-up to [47907].

See #48529.

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