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

Add context property mapping to block registration #23180

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Jun 15, 2020

Fixes #23179

Description

Block context was not being parsed in block registration.

This broke dynamic blocks which relied on $block->context. The context properties were not being added to context from available_context because it only adds properties which existed in the block metadata... and context was not parsed in

How has this been tested?

Locally.

Screenshots

Before:

Screen Shot 2020-06-15 at 11 32 11 AM

After: (ignore the echo; it was for debugging)

Screen Shot 2020-06-15 at 12 45 01 PM

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@noahtallen noahtallen added [Feature] Block API API that allows to express the block paradigm. [Type] Regression Related to a regression in the latest release labels Jun 15, 2020
@noahtallen noahtallen self-assigned this Jun 15, 2020
@noahtallen noahtallen changed the title Add context property mapping Add context property mapping to block registration Jun 15, 2020
@github-actions
Copy link

Size Change: 0 B

Total Size: 1.13 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.27 kB 0 B
build/block-directory/style-rtl.css 892 B 0 B
build/block-directory/style.css 892 B 0 B
build/block-editor/index.js 106 kB 0 B
build/block-editor/style-rtl.css 12.1 kB 0 B
build/block-editor/style.css 12.1 kB 0 B
build/block-library/editor-rtl.css 7.88 kB 0 B
build/block-library/editor.css 7.89 kB 0 B
build/block-library/index.js 129 kB 0 B
build/block-library/style-rtl.css 7.96 kB 0 B
build/block-library/style.css 7.96 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 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 48.1 kB 0 B
build/components/index.js 195 kB 0 B
build/components/style-rtl.css 19.5 kB 0 B
build/components/style.css 19.5 kB 0 B
build/compose/index.js 9.32 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 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 568 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 975 B 0 B
build/edit-navigation/style.css 974 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 5.6 kB 0 B
build/edit-post/style.css 5.6 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/index.js 9.34 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 423 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 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 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 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/media-utils/index.js 5.29 kB 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.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 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.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Works like a charm! Thanks Noah 🙇

@noahtallen
Copy link
Member Author

trying to get the tests to pass. it shouldn't be related at all

@noahtallen
Copy link
Member Author

wtf travis

@noahtallen noahtallen merged commit 23433a6 into master Jun 16, 2020
@noahtallen noahtallen deleted the fix/server-block-context branch June 16, 2020 00:34
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 16, 2020
@@ -169,6 +169,7 @@ function register_block_type_from_metadata( $file_or_folder, $args = array() ) {
$property_mappings = array(
'title' => 'title',
'category' => 'category',
'context' => 'context',
Copy link
Member

Choose a reason for hiding this comment

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

I guess we need to add providesContext, too :)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in #23212

@spacedmonkey
Copy link
Member

@ockham Why wasa this merged. Either of the REST API team signed off and this change is wrong. REST API already has a context value and can be used for something else.

@gziolo
Copy link
Member

gziolo commented Jun 16, 2020

At the moment, it's correct since Gutenberg relays on context and providesContext fields in WP_Block_Type instance. We should address it separately.

@ockham
Copy link
Contributor

ockham commented Jun 16, 2020

@ockham Why wasa this merged. Either of the REST API team signed off and this change is wrong. REST API already has a context value and can be used for something else.

My apologies, I might not be very familiar with this code, but the fix seemed to make sense and fixed the issue we were seeing. I also wasn't aware it affected the REST API, or that it needed sign-off by the REST API team. Could you elaborate why the change is wrong, and what policy we should observe with regard to sign-off by individual teams?

@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
@noahtallen
Copy link
Member Author

noahtallen commented Jun 16, 2020

Why was this merged

Travis passed and this fixes a bug where blocks would not render on the front end if they relied on block context. (the change which introduced the property mapping mechanism basically stopped a block from registering the context it needs.) So it's more of a hotfix :)

Maybe we need more strict checks to make sure that PRs affecting API are not merged without sign-off from the API team?

@noahtallen
Copy link
Member Author

So what's the follow-up here? It looks like we might need to:

  1. Add providesContext to the property mapping
  2. Change the property mapping for context to something other than context. (And then update internal use.)

@noahtallen
Copy link
Member Author

@spacedmonkey name fixed in #23212

This was referenced Jun 24, 2020
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] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FSE: Front-end block context does not exist
4 participants