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 Library: Register all block with "block.json" on the server' #22491

Merged
merged 2 commits into from
May 27, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 20, 2020

Description

This PR registers on the server all core blocks that have block.json defined when they don't contain PHP files that would take care of it. This way soon we should be able to get a few benefits like:

  1. Ability to expose all registered blocks and their definitions with a new REST API endpoint as proposed in Add block types REST API. #21065 by @spacedmonkey.
  2. Using the server-side preloaded REST API call to the endpoint mentioned in (1) we should be able to finally remove the temporary workaround used here:
    https://github.com/WordPress/wordpress-develop/blob/437ac63d7652b08095dc7f77b6ba4ef52db44dbb/src/wp-admin/edit-form-blocks.php#L111-L115
  3. A way to process all core blocks on the server using their definitions.

How has this been tested?

I checked in the source code that included blocks are listed in the wp.blocks.unstable__bootstrapServerSideBlockDefinitions call:

Screen Shot 2020-05-20 at 16 04 47

Types of changes

New feature (non-breaking change which adds functionality).

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 self-assigned this May 20, 2020
@github-actions
Copy link

github-actions bot commented May 20, 2020

Size Change: 0 B

Total Size: 1.12 MB

ℹ️ 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 3.39 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.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.17 kB 0 B
build/block-library/editor.css 7.17 kB 0 B
build/block-library/index.js 119 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 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 48.1 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.24 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.42 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.11 kB 0 B
build/edit-navigation/index.js 6.6 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.8 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 7.73 kB 0 B
build/edit-widgets/style-rtl.css 4.59 kB 0 B
build/edit-widgets/style.css 4.59 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.6 kB 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 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.64 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.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.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.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

@gziolo gziolo force-pushed the update/server-side-register-blocks branch from 272ae3e to d3b0211 Compare May 20, 2020 11:11
@gziolo gziolo added [Package] Block library /packages/block-library [Type] Task Issues or PRs that have been broken down into an individual action to take [Status] In Progress Tracking issues with work in progress labels May 20, 2020
@gziolo gziolo requested a review from oandregal May 20, 2020 13:50
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label May 20, 2020
@gziolo gziolo marked this pull request as ready for review May 20, 2020 14:01
@spacedmonkey
Copy link
Member

@gziolo Do you want to wait until this is merged then I can update my PR at #21065 to expose all the fields in the API?

@gziolo gziolo requested review from aduth and azaozz May 20, 2020 14:09
@gziolo
Copy link
Member Author

gziolo commented May 21, 2020

This is the list of blocks

  • Archives (register_block_type_from_metadata called in archives.php that is required directly)
  • Audio
  • Block (block.php)
  • Button
  • buttons
  • Calendar (calendar.php)
  • Categories (categories.php)
  • Classic
  • Code
  • Column
  • Columns
  • Cover (cover.php)
  • Embed – no block.json!
  • File
  • Gallery
  • Group
  • Heading
  • HTML
  • Image
  • Latest Comments (latest-comments.php)
  • Latest Posts (latest-posts.php)
  • Legacy Widget (legacy-widget.php)
  • List
  • Media Text
  • Missing
  • More
  • Navigation (navigation.php)
  • Navigation Link
  • Next Page
  • Paragraph
  • Post Author (post-author.php)
  • Post Comments (post-comments.php)
  • Post Comments Count (post-comments-count.php)
  • Post Comments Form (post-comments-form.php)
  • Post Content (post-content.php)
  • Post Date (post-date.php)
  • Post Excerpt (post-excerpt.php)
  • Post Featured Image (post-featured-image.php)
  • Post Tags (post-tags.php)
  • Post Title (post-title.php)
  • Preformatted
  • Pullquote
  • Query (query.php)
  • Query Loop (query-loop.php)
  • Query Pagination (query-pagination.php)
  • Quote
  • RSS (rss.php)
  • Search (search.php)
  • Separator
  • Shortcode (shortcode.php)
  • Site Title (site-title.php)
  • Social Link (social-link.php)
  • Social Links
  • Spacer
  • Subhead
  • Table
  • Tag Cloud (tag-cloud.php)
  • Template Part (template-part.php)
  • Text Columns
  • Verse
  • Video
  • Widget Area

@gziolo gziolo force-pushed the update/server-side-register-blocks branch from d3b0211 to 446d52c Compare May 21, 2020 08:30
@gziolo
Copy link
Member Author

gziolo commented May 21, 2020

With 446d52c I added all missing blocks but Embed block. It has to be tackled separately because it's in fact 30-40 blocks that are nearly identical. They should be converted into a single block with multiple variations. @mcsf did you start some work towards that?

@gziolo
Copy link
Member Author

gziolo commented May 21, 2020

I discussed Widgets Area block with @jorgefilipecosta and it seems like it doesn't have to be loaded in every context. I'm sure the same applies to many other blocks added recently. @youknowriad and @mtias do you have some strategy planned moving forward in terms of whether to limit what types of blocks are loaded depending on the edit page type?

@aduth, while working on updates to block.json files I realized that some blocks already use context implementation, but they don't define parent property. Do you this there should be some sort of ancestor relationship enforced to prevent inserting those blocks when they can't read such context?

@aduth
Copy link
Member

aduth commented May 21, 2020

@aduth, while working on updates to block.json files I realized that some blocks already use context implementation, but they don't define parents property. Do you this there should be some sort of ancestor relationship enforced to prevent inserting those blocks when they can't read such context?

The short answer for me would be "no".

But I do grant that there's an interesting problem here in if and how a block should assume for context to have been provided. Part of the reason that context even exists as a property (vs. all available context as some sort of grab bag) is so that the framework can have some awareness of what context a block is expecting, and be able to make accommodations based on that. Specifically I can see this being used in the sort of scenario you describe where framework can deal with the problem that the context needs are not satisfied.

From the block's perspective, it's still an open question if it should assume the context value is available or not, which could be affected by the prior handling. Currently, they do handle this, though in a way that's not especially graceful (example, returning empty string).

It might be something to observe over time:

Finally, it's worth clarifying: Context doesn't necessarily come from the direct parent. It can come anywhere from the ancestry. So parents isn't really relevant for context in a lot of cases.

@gziolo
Copy link
Member Author

gziolo commented May 22, 2020

To make it clear for all observers, all that discussion I started with @aduth isn't a blocker to this particular PR.

@aduth, thank you for your detailed answer. It looks like there are many ways to work with a context in the case when it's not provided in the upper scope of the tree where block lives.

Finally, it's worth clarifying: Context doesn't necessarily come from the direct parent. It can come anywhere from the ancestry. So parents isn't really relevant for context in a lot of cases.

I was aware of this limited usage of parents property. Still, it raises the question of whether we should be concerned that there is no way to remove from the inserter a block if it has to exist inside another block at any level of nesting. It doesn't necessarily have to be related to the existence of context property.


One more question, do you envision any issues on the frontend caused by the block registration process happening on the server knowing that we don't run the same logic that hooks operating on supports (align, className, etc.) do on the frontend? (It's hard to phrase it clearly...)

@mcsf
Copy link
Contributor

mcsf commented May 22, 2020

With 446d52c I added all missing blocks but Embed block. It has to be tackled separately because it's in fact 30-40 blocks that are nearly identical. They should be converted into a single block with multiple variations. @mcsf did you start some work towards that?

Regrettably no, what little work I had remained in a local branch and I had to deprioritise that task.

@aduth
Copy link
Member

aduth commented May 22, 2020

One more question, do you envision any issues on the frontend caused by the block registration process happening on the server knowing that we don't run the same logic that hooks operating on supports (align, className, etc.) do on the frontend? (It's hard to phrase it clearly...)

I don't really imagine there should be. For the most part, if a block type doesn't have an associated render_callback, there's not really much consideration of it (including attributes and supports) in the front-end rendering.

@@ -16,6 +16,41 @@ function gutenberg_reregister_core_block_types() {
return;
}

$block_folders = array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of an explicit array, can we just loop through the folder?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the ideal world, I would do it, but some blocks are behind the flag and other call register_block_from_metadata in PHP file. Well, we could detect the latter, and blacklist the former. Hmmm, I might open a follow-up. I guess I could use glob right?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also this proposed addition to WordPress core from @aduth in https://core.trac.wordpress.org/ticket/49615:
register_block_type_args filter

With that, we could find all block.json file to register blocks and use this hook to set render_callback separately.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Do you think we should create a trac ticket to backport the API and this change?

@gziolo
Copy link
Member Author

gziolo commented May 27, 2020

Do you think we should create a trac ticket to backport the API and this change?

There is still some work going on in #22491 but I agree that we should at least have a tracking ticker in Trac 👍

@gziolo gziolo merged commit a271af4 into master May 27, 2020
@gziolo gziolo deleted the update/server-side-register-blocks branch May 27, 2020 08:31
@gziolo
Copy link
Member Author

gziolo commented May 27, 2020

Filed a follow-up #22660 for the only remaining block – Embed.

@gziolo
Copy link
Member Author

gziolo commented May 27, 2020

Trac ticket created as well: https://core.trac.wordpress.org/ticket/50263.

@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
@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
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block library /packages/block-library [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants