Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block API: Preserve unknown, respect null in server attributes preparation #12003

Merged
merged 4 commits into from
Nov 20, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Nov 16, 2018

This pull request seeks to revise the behavior of the server-side BlockType::prepare_attributes_for_render with regards to validation:

  • Skip validation where there is no attribute definition, but keep the attribute value
    • Previously, the attribute would be omitted from the attributes passed to render_callback.
    • Notably, this resolves an issue where render_callback cannot receive a block's align and customClassName attribute values, since these are defined as a client-side filter (related).
  • Validate null as a proper value in its own right
    • Previously, a client implementation of a block could track {"attribute":null} as an explicitly empty value, and the server would wrongly initiate defaulting behavior.
    • The new behavior will now only populate a default value if the attribute is not defined at all, including when unset in its being invalid per the attribute schema.

This is partly intended to serve for consistency:

  • The prior implementation already had a shortcutting condition to return the attributes verbatim if there were no attributes defined. An unknown attribute would be kept, but only if the block type had no attributes.
  • Omitting the invalid attribute type when no default exists aligns with the client implementation
    • e.g. wp.blocks.parse( '<!-- wp:latest-posts {"className":2} /-->')[ 0 ].attributes.className === undefined

One potential downside is a newly introduced inconsistency in that the client still does not respect unknown attributes. This may be simple enough to add support if desired.

Testing instructions:

Ensure PHP tests pass:

npm run test-php phpunit/class-block-type-test.php 

Optionally, you may try registering server-side a render_callback for a block which supports alignment or custom class name in the client, ensuring that the value is passed to the render_callback.

Possible future tasks:

cc @mtias @azaozz

@aduth aduth added [Feature] Block API API that allows to express the block paradigm. Framework Issues related to broader framework topics, especially as it relates to javascript labels Nov 16, 2018
@aduth
Copy link
Member Author

aduth commented Nov 16, 2018

Funny enough, I caught myself staring at @azaozz 's #11973 wondering: How does render_callback work at all? If all the attributes are defined in the client and thus unknown to the server, and without this pull request we omit unknown attributes, shouldn't then render_callback receive no attributes at all?

Well, in fact, this ties back to the following note of the original comment:

The prior implementation already had a shortcutting condition to return the attributes verbatim if there were no attributes defined. An unknown attribute would be kept, but only if the block type had no attributes.

So a render_callback would receive the attributes, but only because it doesn't define attributes at all. This feels like a very fragile and inconsistent behavior, since we're very much not omitting unknown attributes as the PHPDoc would have you believe, at least not consistently.

In master, you'd find the following, which appears nonsensical to me:

( new WP_Block_Type( 'core/dummy', [] ) )->prepare_blocks_for_attributes( [ 'exists' => 'keep' ] );
// [ 'exists' => 'keep' ]

( new WP_Block_Type( 'core/dummy', [ 'attributes' => [] ] ) )->prepare_blocks_for_attributes( [ 'exists' => 'keep' ] );
// []

@chrisvanpatten
Copy link
Contributor

Notably, this resolves an issue where render_callback cannot receive a block's align and customClassName attribute values, since these are defined as a client-side filter (related).

This is huge. Right now you have to (annoyingly) register align and className as attributes on your server-rendered blocks manually, which is obviously inconsistent from the JS representation.

It would be nice if these (and any other "protected" properties) could be validated on the server as well, but that's a discussion for the future.

This feels like a very fragile and inconsistent behavior, since we're very much not omitting unknown attributes as the PHPDoc would have you believe, at least not consistently.

You can say that again!

Figuring out how server-registered attributes worked was a huge time sink for me when we first started building blocks. This seems like a huge step forward into making this behaviour more consistent and easily understandable.

@aduth aduth added this to the 4.5 milestone Nov 19, 2018
Previously we would pass `NULL` to `render`. While `render` provides a default `array()` value, since an explicit value is provided, it would not take effect. With this change, the endpoint assures a default value to be provided as a parameter to get the `get_item` callback that is compatible with the function signature of `WP_BlockType::render`.

Alternatively, it could be considered to have separate invocations of `render`, depending on whether the request parameter is set.
@aduth aduth force-pushed the update/prepare-attributes-respect-unknown branch from c7bb6d6 to eb40939 Compare November 19, 2018 19:06
@@ -169,14 +174,26 @@ function test_prepare_attributes() {
$this->assertEquals(
array(
'correct' => 'include',
'wrongType' => null,
/* wrongType */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this change about?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's indicating that we expect that 'wrongType' will not appear in $prepared_attributes.

@mtias mtias added the [Type] Code Quality Issues or PRs that relate to code quality label Nov 20, 2018
@mtias
Copy link
Member

mtias commented Nov 20, 2018

I agree with this update and the reasoning.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this logic makes a lot more sense to me.

I tested this by registering a render_callback on paragraph blocks and checking that it renders className and align properly.

register_block_type(
	'core/paragraph',
	array(
		'render_callback' => function( $attributes ) {
			return '<p>' . json_encode( $attributes ) . '</p>';
		},
	)
);

@gziolo gziolo merged commit 53c975b into master Nov 20, 2018
@gziolo gziolo deleted the update/prepare-attributes-respect-unknown branch November 20, 2018 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants