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

Blocks: skip null values returned from the server during registration #22849

Merged
merged 2 commits into from
Jun 4, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jun 3, 2020

Description

Fixes #22810.

This PR implements the approach discussed in #22810 and summarized by @aduth in #22810 (comment):

This distinction of registration vs. registry could be important, and also affect how we consider block type storage in the client. I think it'd be reasonable to assume that registration in both PHP and JavaScript support the omission of certain optional properties, but for how that type is represented in the registry of block types, we'd expect that the properties always be assigned, even if to some "empty" default.

For me, I see a few specific points of action:

  1. I do think icon and category should default to null
    • In the former, null could be considered a valid and handled case for rendering an icon. But the client expects that if given a string, the string is of a set of Dashicon slugs, which the empty string '' is not.
    • Likewise, if category is a string, it's assumed to be one of a set of valid categories, which the empty string '' is not.
  2. Especially considering Replace unstable__bootstrapServerSideBlockDefinitions with call to block type REST API #22812, we could expect that we will want to pipe output of a block registry containing all possible block properties into the client-side registration. This could affect one of the following:
    1. Block registration should anticipate null and treat it equivalent to undefined in how it considers fallback / default values.
    2. Whichever mechanism responsible for piping that data should unset null values before providing it to the registration function.
  3. To bring consistency between how block types are stored server-side and client-side, the client-side should also always assign all known properties of the block type in storage (or at least block type selectors), if not already done.

While it doesn't affect the client-side implementation very much, I'd also wonder about editor_script, script, editor_style, and style being assigned to empty strings as defaults. In general, it feels that null is the better representation of an empty value. As seen already, if provided as the same type in which its non-empty behavior is expected (e.g. a script handle), it can cause some breakage (e.g. this issue with icon as empty string).

Summary

REST API fields that default now to null (all of them are optional) in addition to other fields covered earlier:

  • description
  • icon
  • category
  • editor_script
  • script
  • editor_style
  • style

Added default values for two more settings in registerBlockType to align with REST API and the server implementation:

  • supports{}
  • styles – []

Updated registerBlockType to skip fields with null and undefined values passed from the server through unstable__bootstrapServerSideBlockDefinitions (that is eventually to be replaced with REST API endpoint).

How has this been tested?

npm run test-unit
npm run test-php

Types of changes

Bug fix (non-breaking change which fixes an issue).

Checklist:

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

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. [Package] Blocks /packages/blocks labels Jun 3, 2020
@gziolo gziolo requested review from spacedmonkey and aduth June 3, 2020 06:15
@gziolo gziolo self-assigned this Jun 3, 2020
@github-actions
Copy link

github-actions bot commented Jun 3, 2020

Size Change: +25 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/blocks/index.js 48.2 kB +25 B (0%)
ℹ️ 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 6.75 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 11.4 kB 0 B
build/block-editor/style.css 11.4 kB 0 B
build/block-library/editor-rtl.css 7.87 kB 0 B
build/block-library/editor.css 7.88 kB 0 B
build/block-library/index.js 126 kB 0 B
build/block-library/style-rtl.css 7.69 kB 0 B
build/block-library/style.css 7.68 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/components/index.js 193 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.33 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.46 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 8.25 kB 0 B
build/edit-navigation/style-rtl.css 878 B 0 B
build/edit-navigation/style.css 876 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 5.43 kB 0 B
build/edit-post/style.css 5.43 kB 0 B
build/edit-site/index.js 15 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 8.83 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 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.7 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.65 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 621 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.52 kB 0 B
build/keycodes/index.js 1.94 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/media-utils/index.js 5.3 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.8 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

@gziolo
Copy link
Member Author

gziolo commented Jun 3, 2020

Changes required in WordPress core added here https://core.trac.wordpress.org/ticket/48529#comment:22:

Index: src/wp-includes/class-wp-block-type.php
===================================================================
--- src/wp-includes/class-wp-block-type.php	(revision 47891)
+++ src/wp-includes/class-wp-block-type.php	(working copy)
@@ -32,9 +32,9 @@
 
 	/**
 	 * @since 5.5.0
-	 * @var string
+	 * @var string|null
 	 */
-	public $category = '';
+	public $category = null;
 
 	/**
 	 * @since 5.5.0
@@ -44,15 +44,15 @@
 
 	/**
 	 * @since 5.5.0
-	 * @var string
+	 * @var string|null
 	 */
-	public $icon = '';
+	public $icon = null;
  
 	/**
 	 * @since 5.5.0
@@ -104,25 +104,25 @@
 	 * Block type editor script handle.
 	 *
 	 * @since 5.0.0
-	 * @var string
+	 * @var string|null
 	 */
-	public $editor_script = '';
+	public $editor_script = null;
 
 	/**
 	 * Block type front end script handle.
 	 *
 	 * @since 5.0.0
-	 * @var string
+	 * @var string|null
 	 */
-	public $script = '';
+	public $script = null;
 
 	/**
 	 * Block type editor style handle.
 	 *
 	 * @since 5.0.0
-	 * @var string
+	 * @var string|null
 	 */
-	public $editor_style = '';
+	public $editor_style = null;
 
 	/**
 	 * Block type front end style handle.
@@ -130,7 +130,7 @@
 	 * @since 5.0.0
 	 * @var string
 	 */
-	public $style = '';
+	public $style = null;
 
 	/**
 	 * Constructor.

lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Jun 4, 2020
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. 



git-svn-id: https://develop.svn.wordpress.org/trunk@47907 602fd350-edb4-49c9-b593-d223f7449a82
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Jun 4, 2020
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. 



git-svn-id: https://develop.svn.wordpress.org/trunk@47907 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jun 4, 2020
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. 


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


git-svn-id: http://core.svn.wordpress.org/trunk@47681 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Jun 4, 2020
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. 


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


git-svn-id: https://core.svn.wordpress.org/trunk@47681 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@gziolo gziolo force-pushed the update/block-defaults-server branch from 8344e7d to e7f44db Compare June 4, 2020 06:14
@gziolo gziolo merged commit 338e253 into master Jun 4, 2020
@gziolo gziolo deleted the update/block-defaults-server branch June 4, 2020 06:47
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone Jun 4, 2020
@oandregal oandregal changed the title Blocks: Skip null values returned from the server during registration Blocks: skip null values returned from the server during registration Jun 8, 2020
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
donmhico pushed a commit to donmhico/wordpress-develop that referenced this pull request Jun 26, 2020
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. 



git-svn-id: https://develop.svn.wordpress.org/trunk@47907 602fd350-edb4-49c9-b593-d223f7449a82
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. [Package] Blocks /packages/blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block API: Registration with "empty" defaults from rWP47875 conflict with block defaults
3 participants