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: Add Block Context support #21467

Merged
merged 54 commits into from
Apr 17, 2020
Merged

Block API: Add Block Context support #21467

merged 54 commits into from
Apr 17, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 7, 2020

Closes #19685
Closes #4671
Previously: #19572
WordPress Trac ticket: https://core.trac.wordpress.org/ticket/49927.

This pull request seeks to implement the means for a block to provide and consume contextual values. In many ways, it is based on the earlier work of #19572, but there are some key differences in the specifics of how context is defined. I've included a fair bit of documentation here as well, which was useful for me in validating ("rubber duck"-ing) the underlying implementation. This documentation should prove useful for understanding what it is, how it works, and why it's proposed to be implemented the way it is.

Differences from #19572:

  • Providing context occurs separately from the block's attributes schema, as its own providesContext property.
  • There's no association from the consumer of where a context value is assumed to be defined. Instead, a provider grants a value via a key corresponding to the name of one of its own attributes, and a consumer references it by name.
  • Server-side, the implementation proposed here does not modify the function signatures of either render_block or a block's render_callback. Instead, both the block context of the current hierarchy and the block itself are assigned as globals which can be referenced in the render_callback. In later revisions of this pull request, an enhancement was achieved where the full $block instance is passed as an argument to render_callback, retaining backward compatibility by implementing ArrayAccess where the instance's array members are short-hand to its own attributes. See Block API: Add Block Context support #21467 (comment) for more information.
    • See documentation
    • For more information and granted downsides, refer to the comment at Server Side Rendering Parent Attributes Context #19685 (comment) .
    • This is future-compatible with revisions to the function signatures, if there's a desire to go this route. I did not feel that this decision should block the implementation. I've remarked to the future possibility of revised function signatures in the above-referenced documentation. Later changes of the pull request implemented said future-compatible revisions.
    • This also solves requirements where some developers have sought to access other properties of $block while within render_callback, avoiding needing to resort to workarounds using render_block hook (e.g. Do advanced rendering by hooking into render_block for navigation block #19991). Later changes of the pull request serve as a solution to this need.
  • Block context is assigned from a provider only from the point of InnerBlocks, as an optimization to avoid establishing a context unnecessarily if there are no children to consume from it.
  • It does not implement any new blocks. For demonstration purposes, the existing Post Title block has been updated to use block context. This was also the case in Block Library: Implement Post block and Post blocks editing. #19572, but currently it is only a static representation of the title, and does not yet support editing.

Open Questions:

  • "Default" context: Is it reasonable to establish the default "post" context? There is certainly prior art for this, given that render_block already manipulates and assigns the $post global. I'd worry if this might lead to a situation where there's not clear expectations around what should or shouldn't be available by default. "Post" is a particularly special case and may be fine to assume it would be unique in this regard.
  • Does context need to be available to save ? I've assumed not, given that this may lead to unintended invalidations, and that at least for the current set of use-cases, context would be evaluated server-side in a render_callback.
  • The comment at Server Side Rendering Parent Attributes Context #19685 (comment) left open the question about whether these keys ought to be namespaced to avoid conflict. While this is still a possibility and the proposed implementation may be overly simplistic (i.e. naive), it may also be fair to say that context will be reserved for very specific usages, and not widely applied enough so as to have high risk for conflict.
  • Minor: Does it make sense that a context object always be provided, even when the block does not define any context to consume? My decision to pass an empty object is largely for developer experience, anticipating potential errors if a developer were to assume that the object is present and attempt unguarded access to properties on that (possibly-undefined) value.

In Progress:

  • I am observing PHP warning notices in the "Site Editor" page related to these changes, which I've yet to resolve. They appear to be related to the assignment of the $block global in the overridden render_block implementation. Fixed, see Block API: Add Block Context support #21467 (comment).

Testing Instructions:

Given that this pull request only ports the Post Title experimental full-site editing block, testing can be a bit cumbersome at the moment, especially if you've not yet defined templates for full-site editing mode.

My recommendation for testing is as follows:

  1. Navigate to Gutenberg > Experiments
  2. Enable "Full Site Editing" and click "Save Changes"
  3. Navigate to Posts > Add New
  4. Insert a Post Title block
  5. Observe that the block inherits the title of the post and updates if you change the title
  6. Preview the post in a separate tab
    • Note: If you don't have templates on your site, you may see a message "No template found". This is expected, and can be disregarded for now.
  7. Navigate to Gutenberg > Experiments
  8. Disable "Full Site Editing" and click "Save Changes"
  9. In your separate preview tab, refresh the page.
  10. Observe that the Post Title block displays the title of the current post

@aduth aduth added [Feature] Block API API that allows to express the block paradigm. [Type] New API New API to be used by plugin developers or package users. labels Apr 7, 2020
@aduth aduth requested a review from youknowriad April 7, 2020 21:09
@github-actions
Copy link

github-actions bot commented Apr 7, 2020

Size Change: +566 B (0%)

Total Size: 842 kB

Filename Size Change
build/block-editor/index.js 105 kB +282 B (0%)
build/block-library/index.js 112 kB +47 B (0%)
build/compose/index.js 6.66 kB +4 B (0%)
build/core-data/index.js 11.4 kB +167 B (1%)
build/edit-post/index.js 27.9 kB +2 B (0%)
build/editor/index.js 43.3 kB +63 B (0%)
build/media-utils/index.js 5.28 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.08 kB 0 B
build/block-library/editor.css 7.09 kB 0 B
build/block-library/style-rtl.css 7.17 kB 0 B
build/block-library/style.css 7.17 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.5 kB 0 B
build/edit-site/style-rtl.css 5.05 kB 0 B
build/edit-site/style.css 5.05 kB 0 B
build/edit-widgets/index.js 7.49 kB 0 B
build/edit-widgets/style-rtl.css 4.67 kB 0 B
build/edit-widgets/style.css 4.67 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/style-rtl.css 3.48 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@noisysocks
Copy link
Member

Nice work! This looks really solid and the API is applicable for things outside of just the Post and Post Content blocks, too. For example, in #21075, we need the Navigation Link block to be able to access styling attributes that are defined by the parent Navigation block.

I've only just caught up on the discussion in #19685, but let me try anyway to review this and answer your questions. (And apologies if I'm slow!)

There's no association from the consumer of where a context value is assumed to be defined. Instead, a provider grants a value via a key corresponding to the name of one of its own attributes, and a consumer references it by name.

I like the balance between simplicity and "correctness" here.

Server-side, the implementation proposed here does not modify the function signatures of either render_block or a block's render_callback. Instead, both the block context of the current hierarchy and the block itself are assigned as globals which can be referenced in the render_callback.

I'm hesitant about adding a new $block global because it's something we will have to forever maintain backwards compatibility for. My preference would be to pass a WP_Block to render_callback. WP_Block can implement ArrayAccess and we can pass a redundant second argument for backwards compatibility.

class WP_Block implements ArrayAccess {
	public $attributes;
	public $content;
	public $context;
	public function offsetGet( $key ) {
		return $attributes[ $key ];
	}
}
...
render_callback( $block, $block->content );

Agree with you that this shouldn't block this PR, though. We have until WP 5.5 to make adjustments to the API.

"Default" context: Is it reasonable to establish the default "post" context? There is certainly prior art for this, given that render_block already manipulates and assigns the $post global. I'd worry if this might lead to a situation where there's not clear expectations around what should or shouldn't be available by default. "Post" is a particularly special case and may be fine to assume it would be unique in this regard.

My only worry here is that block developers may come to assume that postId and postType are always available from context which is not true in the case that their block is displayed in a standalone <BlockEditor /> or in the Widgets and Navigation screens.

Does context need to be available to save ? I've assumed not, given that this may lead to unintended invalidations, and that at least for the current set of use-cases, context would be evaluated server-side in a render_callback.

I think best to leave this out until Gutenberg has a practical use-case for it.

The comment at #19685 (comment) left open the question about whether these keys ought to be namespaced to avoid conflict. While this is still a possibility and the proposed implementation may be overly simplistic (i.e. naive), it may also be fair to say that context will be reserved for very specific usages, and not widely applied enough so as to have high risk for conflict.

Whatever small risk of conflict there is is mitigated by the explicitness of having providesContext and context, in my opinion.

Minor: Does it make sense that a context object always be provided, even when the block does not define any context to consume? My decision to pass an empty object is largely for developer experience, anticipating potential errors if a developer were to assume that the object is present and attempt unguarded access to properties on that (possibly-undefined) value.

Yes I think so. It's nice for block authors to just be able to write edit( { context: { postId } } ).

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

I really like the API and this will help massively for some work I'm undertaking to tidy up the Navigation block (#21075).

I feel like the API works well for blocks that have a very well defined relationship, like Navigation and Navigation Link, the use case here is that colors (particularly background color) can be defined on the parent and propagate to the child.

I've wondered about a future where we wanted to bring something like the Search block into the allowed blocks for Navigation and how something like that might work. Potentially it might have to be a separate block. Variations is also something I considered, whether a variation could use context where the original block might not. Not something to worry about right now, but just some thoughts.

lib/compat.php Outdated Show resolved Hide resolved
lib/compat.php Outdated Show resolved Hide resolved
packages/block-editor/src/components/inner-blocks/index.js Outdated Show resolved Hide resolved
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Excellent work. I like the direction, and I agree that we can start with simplified approach that assumes that exposed data with context by default gets overridden when the same field gets exposed deeper in the tree. The fact that we would use the pair context and providesContext leaves some room for further refinements. Let's see if this ever becomes an issue.

@aduth
Copy link
Member Author

aduth commented Apr 8, 2020

Thanks everyone for the feedback here! I'll work to address each item individually.

@aduth
Copy link
Member Author

aduth commented Apr 8, 2020

Server-side, the implementation proposed here does not modify the function signatures of either render_block or a block's render_callback. Instead, both the block context of the current hierarchy and the block itself are assigned as globals which can be referenced in the render_callback.

I'm hesitant about adding a new $block global because it's something we will have to forever maintain backwards compatibility for. My preference would be to pass a WP_Block to render_callback. WP_Block can implement ArrayAccess and we can pass a redundant second argument for backwards compatibility.

class WP_Block implements ArrayAccess {
	public $attributes;
	public $content;
	public $context;
	public function offsetGet( $key ) {
		return $attributes[ $key ];
	}
}
...
render_callback( $block, $block->content );

Agree with you that this shouldn't block this PR, though. We have until WP 5.5 to make adjustments to the API.

@noisysocks I might not be understanding correctly, but is there something about WP_Block implementing ArrayAccess that would help us get around the fact that we already have a render_callback for which we pass the attributes as the first argument?

I would be happy if we can find a way to integrate this into the function signature of render_callback. Historically, the sticking point has been: We already have a function signature, we (presumably) can't change it except to add new arguments, and adding those new arguments would be redundant.

I've personally felt this is partly what's blocked an implementation thus far.

The choice to use a global is in many ways intended to be consistent with the existence of a $post global. Neither is particularly ideal. Noted also in the original comment, I do think there's a future-compatibility to enable us to change this in the future. You're right to point out that we'd still be committed to maintaining backward-compatibility with the global. I'm not sure how much maintenance overhead this would actually bring, however.

@aduth
Copy link
Member Author

aduth commented Apr 8, 2020

"Default" context: Is it reasonable to establish the default "post" context? There is certainly prior art for this, given that render_block already manipulates and assigns the $post global. I'd worry if this might lead to a situation where there's not clear expectations around what should or shouldn't be available by default. "Post" is a particularly special case and may be fine to assume it would be unique in this regard.

My only worry here is that block developers may come to assume that postId and postType are always available from context which is not true in the case that their block is displayed in a standalone <BlockEditor /> or in the Widgets and Navigation screens.

It's a fair worry. It ties in to a historical desire to try to limit assumptions from blocks about it being within the context of some post, at least where appropriate (e.g. a paragraph is static, self-contained, and needn't worry about the existence of a post). Conversely, some blocks we've made the conscious decision that this contextual awareness is appropriate, like in Navigation block, or new full-site editing blocks like "Post Title". These will need to be aware of some external factors like the current post, or site settings.

To your overall point, I think we should generally want to continue to push developers to consider absence of context as a default position, and make a conscious choice to become "aware" only as appropriate.

Whether we've put sufficient safeguards / guidelines in place to ensure this happens is yet to be seen, and could be further improved (documentation, technical restrictions, etc).

@aduth
Copy link
Member Author

aduth commented Apr 9, 2020

One other thing I failed to mention: This requires that there be upstream revisions to the following function:

https://github.com/WordPress/wordpress-develop/blob/3df79c3/src/wp-admin/includes/post.php#L2216-L2246

Since we are now adding new settings properties context and providesContext in the current proposal.

It works okay in this pull request, largely because we're importing block.json both in the server and in the client, and thus not depending on this server-side bootstrapping. This won't always be the case, depending if a plugin registers a block using register_block_type directly.

These keys aren't filterable, so there's no way to extend the set from Gutenberg. An upstream ticket should propose to add new properties (once this proposal is settled), and optionally make them filterable.

@aduth aduth requested review from nerrad and ntwb as code owners April 10, 2020 14:09
@aduth
Copy link
Member Author

aduth commented Apr 10, 2020

Status update:

  • There's been some revisions to how context is provided, related to the conversation at Block API: Add Block Context support #21467 (comment) . I'd appreciate any thoughts on whether these changes are sensible given the needs discussed around scoping and remapping context.
  • I still haven't tackled the Site Editor warning notices mentioned in the original comment. I also need to dig in to some of build failures, which may or may not be simple fixes.
  • Once things start to settle, I want to create tracking issues for many of the follow-up tasks which have been discussed in the comments.
    • I've also had to resort to some interesting workarounds (e.g. 7de9e83) which could ideally be addressed/improved separately to this pull request.

@aduth
Copy link
Member Author

aduth commented Apr 22, 2020

While it was the intention to retain full backward-compatibility, there have been observed some incompatibilities resulting from the changes here to render_callback.

I've summarized the problem and potential solutions at #21797.

@gziolo
Copy link
Member

gziolo commented Jun 17, 2020

Noting that I added WordPress Core Trac ticket to the description: https://core.trac.wordpress.org/ticket/49927.

@gziolo gziolo added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 18, 2020
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Jun 25, 2020
Backports functionality added in Gutenberg in the following PRs:
- WordPress/gutenberg#21467
- WordPress/gutenberg#21925
It's a few ideas related to block rendering and the provided block value, which is particularly impactful for work around block context.

Props aduth, TimothyBJacobs, noisysocks, epiqueras, youknowriad, talldanwp, zebulan.
Fixes #49926.



git-svn-id: https://develop.svn.wordpress.org/trunk@48159 602fd350-edb4-49c9-b593-d223f7449a82
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Jun 25, 2020
Backports functionality added in Gutenberg in the following PRs:
- WordPress/gutenberg#21467
- WordPress/gutenberg#21925
It's a few ideas related to block rendering and the provided block value, which is particularly impactful for work around block context.

Props aduth, TimothyBJacobs, noisysocks, epiqueras, youknowriad, talldanwp, zebulan.
Fixes #49926.



git-svn-id: https://develop.svn.wordpress.org/trunk@48159 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jun 25, 2020
Backports functionality added in Gutenberg in the following PRs:
- WordPress/gutenberg#21467
- WordPress/gutenberg#21925
It's a few ideas related to block rendering and the provided block value, which is particularly impactful for work around block context.

Props aduth, TimothyBJacobs, noisysocks, epiqueras, youknowriad, talldanwp, zebulan.
Fixes #49926.


Built from https://develop.svn.wordpress.org/trunk@48159


git-svn-id: http://core.svn.wordpress.org/trunk@47928 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Jun 25, 2020
Backports functionality added in Gutenberg in the following PRs:
- WordPress/gutenberg#21467
- WordPress/gutenberg#21925
It's a few ideas related to block rendering and the provided block value, which is particularly impactful for work around block context.

Props aduth, TimothyBJacobs, noisysocks, epiqueras, youknowriad, talldanwp, zebulan.
Fixes #49926.


Built from https://develop.svn.wordpress.org/trunk@48159


git-svn-id: https://core.svn.wordpress.org/trunk@47928 1a063a9b-81f0-0310-95a4-ce76da25c4cd
donmhico pushed a commit to donmhico/wordpress-develop that referenced this pull request Jun 26, 2020
Backports functionality added in Gutenberg in the following PRs:
- WordPress/gutenberg#21467
- WordPress/gutenberg#21925
It's a few ideas related to block rendering and the provided block value, which is particularly impactful for work around block context.

Props aduth, TimothyBJacobs, noisysocks, epiqueras, youknowriad, talldanwp, zebulan.
Fixes #49926.



git-svn-id: https://develop.svn.wordpress.org/trunk@48159 602fd350-edb4-49c9-b593-d223f7449a82
@ellatrix ellatrix mentioned this pull request Jul 3, 2020
12 tasks
@gziolo gziolo removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 8, 2020
@gziolo gziolo mentioned this pull request Mar 11, 2021
16 tasks
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. [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server Side Rendering Parent Attributes Context Pass the block name to the render callback
6 participants