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.

Sign up for GitHub

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

Jump to bottom

Introduce block metadata files for dynamic blocks #14238

Closed
wants to merge 6 commits into from
Closed

Introduce block metadata files for dynamic blocks #14238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5c05cd8
Introduce block metadata files for dynamic blocks
gziolo Mar 5, 2019
7519ab6
Add two more defintions for dynamic blocks
gziolo Mar 5, 2019
33a6265
Convert all remaining dynamic blocks to use metadata format
gziolo Mar 5, 2019
625c7e7
Add a way to override core block registration based on metadata
gziolo Mar 5, 2019
cff3053
Convert Shortcode block to use metadata format
gziolo Mar 6, 2019
981e136
Convert Search block to use metadata format
gziolo Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions lib/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php
/**
* Overrides all core blocks.
*
* @package gutenberg
*/

/**
* Registers block type from metadata file.
*
* @param string $file Path to the translation file to load.
*/
function gutenberg_register_block_type_from_metadata( $file ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we see becoming a part of core implementation? I think for any new functions like this, I'm wondering if we should bother with the namespacing convention, since it was a pain point for deprecating Gutenberg-specific functions toward and after WordPress 5.0.

Wondering if instead it's enough to just if ( ! function_exists( 'register_block_type_from_metadata' ) ) {

Copy link
Member Author

Choose a reason for hiding this comment

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

I had it named this way initially. I wasn't sure what the process looks like so I opted for gutenberg_ prefixed version. I'm fine with updating it as suggested as soon as we figure out how this definition should look and work like.

$metadata = json_decode( file_get_contents( $file ), true );
Copy link
Member

Choose a reason for hiding this comment

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

Technically this implementation seems to be fine for a file which doesn't exist (file_get_contents returns false) or a file which is invalid JSON (json_decode returns false), only because empty is very tolerant about what it receives. Could be a potential source of error in the future, at least if not tested against.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should use more checks with proper error messages. I will make those checks more granular 👍

Copy link
Member

Choose a reason for hiding this comment

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

Also make sure to use wp_json_encode() :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Usages of file_get_contents() in Core are typically prefix with an error control operator @file_get_conents() and there are more resilient read functions in the WP_Filesystem class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sure this part needs more work if we decide to introduce such API for reading JSON file. This PR is more exploratory. I expect it's going to be divided in more solid smaller PRs which will implement more granular features. If someone wants to work on this part, we can find ways to split this task :)

if ( empty( $metadata['name'] ) || empty( $metadata['title'] ) || empty( $metadata['category'] ) ) {
$message = __( 'Required block types must be provided.' );
_doing_it_wrong( __FUNCTION__, $message, '5.2.0' );
return;
}
$block_name = $metadata['name'];
$settings = array(
'title' => $metadata['title'],
'category' => $metadata['category'],
);
foreach( array( 'description', 'keywords', 'attributes', 'supports' ) as $field_name ) {
if ( ! empty( $metadata[ $field_name ] ) ) {
$settings[ $field_name ] = $metadata[ $field_name ];
}
}
if ( ! empty( $metadata['renderCallback'] ) ) {
$render_callback_file = dirname( $file ) . '/' . $metadata['renderCallback'];
if ( file_exists( $render_callback_file ) ) {
require $render_callback_file;
$settings['render_callback'] = 'gutenberg_render_block_' . str_replace( array( '/', '-' ), '_', $block_name );
Copy link
Member

Choose a reason for hiding this comment

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

I think it requires anonymous functions (PHP 5.4.0+), but I wonder if rather than relying on a specific naming convention for functions, we use the file returns semantics:

https://gist.github.com/aduth/ef27bfaae19a9959a263f1600b37c71d

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think too much about so far. I definitely would prefer to avoid using the specific conventions for naming functions. Another solution is to use a function which registers such callbacks as follows:

register_block_render_callback( 'core/block', 'render_block_core_block' );

which would be placed inside a PHP file which would also contain the definition. We would also have to change the way such file is required. Maybe we load metadata definitions in one go. Then at the time, we are about to process render_callbacks for all registered blocks we require all files referenced as renderCallbacks?

Copy link
Member

Choose a reason for hiding this comment

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

The register_block_render_callback could impose more requirements on the runtime which consumes from the file (i.e. the function needs to be defined somewhere), in case that might deter us.

A yet-other idea is treating it more like a template file, expecting it to output HTML directly. I'm not sure this is a good way to go though, and it's not obvious how the template would read from attributes (unless they were just some $attributes global defined for use by the file).

Copy link
Member Author

Choose a reason for hiding this comment

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

Some of those existing render callbacks reference other custom functions so this might get tricky. I don't know, maybe this would work. I'd love to see some thoughts from developers better familiar with patterns used in other parts of WordPress core like @pento or @danielbachhuber.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure I follow. Why is $metadata['renderCallback'] a file and not simply a callable function?

Probably the best example of prior art to follow is register_shortcode() (which accepts a callable function). I wouldn't be opposed to taking the template file approach instead, but core doesn't currently have the pattern of injecting variables into a template (see the long conversation in 21676)

}
}
$registry = WP_Block_Type_Registry::get_instance();
if ( $registry->is_registered( $block_name ) ) {
$registry->unregister( $block_name );
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 this is part of it "overriding" ? Not something to be included with a final implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely, it was only added to make it work until it's fixed in master :)

}
register_block_type( $block_name, $settings );
}

function gutenberg_override_core_blocks() {
$block_types = array(
'archives',
'block',
'calendar',
'categories',
'latest-comments',
'latest-posts',
'rss',
'search',
'shortcode',
'tag-cloud'
);

$file_format = dirname( dirname( __FILE__ ) ) . '/packages/block-library/src/%s/block.json';
foreach ( $block_types as $name ) {
gutenberg_register_block_type_from_metadata( sprintf( $file_format, $name ) );
}
}

add_action( 'init', 'gutenberg_override_core_blocks' );
36 changes: 1 addition & 35 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,38 +20,4 @@
require dirname( __FILE__ ) . '/register.php';
require dirname( __FILE__ ) . '/demo.php';
require dirname( __FILE__ ) . '/widgets-page.php';

// Register server-side code for individual blocks.
if ( ! function_exists( 'render_block_core_archives' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/archives/index.php';
}
if ( ! function_exists( 'render_block_core_block' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/block/index.php';
}
if ( ! function_exists( 'render_block_core_categories' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/categories/index.php';
}
// Currently merged in core as `gutenberg_render_block_core_latest_comments`,
// expected to change soon.
if ( ! function_exists( 'render_block_core_calendar' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/calendar/index.php';
}
if ( ! function_exists( 'render_block_core_latest_comments' )
&& ! function_exists( 'gutenberg_render_block_core_latest_comments' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/latest-comments/index.php';
}
if ( ! function_exists( 'render_block_core_latest_posts' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/latest-posts/index.php';
}
if ( ! function_exists( 'render_block_core_rss' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/rss/index.php';
}
if ( ! function_exists( 'render_block_core_shortcode' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/shortcode/index.php';
}
if ( ! function_exists( 'render_block_core_tag_cloud' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/tag-cloud/index.php';
}
if ( ! function_exists( 'render_block_core_search' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/search/index.php';
}
require dirname( __FILE__ ) . '/blocks.php';
32 changes: 32 additions & 0 deletions packages/block-library/src/archives/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"name": "core/archives",
"title": "Archives",
"category": "widgets",
"icon": {
"src": "icon.js"
Copy link
Member

Choose a reason for hiding this comment

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

This one kind of sticks out as far as being inconsistent with other file references like in edit and renderCallback. I see from #13693 this is done as such to support properties like foreground and background color assignments.

In all, I think it's okay, though I also wonder on a few variations:

  • Optionally also allow icon to be assigned as a string if it only needs to assign src
  • Create a pattern around "file reference with options", something like a tuple of [ file, options ] ({ "icon": [ "icon.js", { "background": "red" } ] })

Was looking at some similar specifications, where src is a pretty accepted convention for naming this type of resource (example).

Copy link
Member Author

Choose a reason for hiding this comment

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

Optionally also allow icon to be assigned as a string if it only needs to assign src

It is reserved for the slug at the moment, which aligns with how you can register block's icon as of today. I would be happy to use the file name as a shortcut but then we need to figure out how to provide the slug differently.

Copy link
Member

Choose a reason for hiding this comment

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

It is reserved for the slug at the moment, which aligns with how you can register block's icon as of today. I would be happy to use the file name as a shortcut but then we need to figure out how to provide the slug differently.

Oh right. That makes sense. I guess the other option is to inspect the shape of the string to see if it "appears" as a file path (e.g. has file extension, or starts with ./)

},
"description": "Display a monthly archive of your posts.",
"attributes": {
"align": {
"type": "string"
},
"className": {
"type": "string"
},
"displayAsDropdown": {
"type": "boolean",
"default": false
},
"showPostCounts": {
"type": "boolean",
"default": false
}
},
"edit": "edit.js",
"supports": {
"align": true,
"alignWide": false,
"html": false
},
"renderCallback": "index.php"
}
13 changes: 1 addition & 12 deletions packages/block-library/src/archives/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,11 @@ import { __ } from '@wordpress/i18n';
*/
import {
InspectorControls,
BlockAlignmentToolbar,
BlockControls,
ServerSideRender,
} from '@wordpress/editor';

export default function ArchivesEdit( { attributes, setAttributes } ) {
const { align, showPostCounts, displayAsDropdown } = attributes;
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we could extract separately? Can we not implement supports.align for server-registered blocks currently?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will tackle it and some other preparatory tasks in their own PRs. I'm using this PR to identify all work that is required to get us closer to have the ability to register blocks with metadata. However, I'm discovering that we have some issues with the existing blocks. className and align is often not defined on the server and therefore not exposed for render_callback method.

const { showPostCounts, displayAsDropdown } = attributes;

return (
<Fragment>
Expand All @@ -38,15 +36,6 @@ export default function ArchivesEdit( { attributes, setAttributes } ) {
/>
</PanelBody>
</InspectorControls>
<BlockControls>
<BlockAlignmentToolbar
value={ align }
onChange={ ( nextAlign ) => {
setAttributes( { align: nextAlign } );
} }
controls={ [ 'left', 'center', 'right' ] }
/>
</BlockControls>
<Disabled>
<ServerSideRender block="core/archives" attributes={ attributes } />
</Disabled>
Expand Down
8 changes: 8 additions & 0 deletions packages/block-library/src/archives/icon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* WordPress dependencies
*/
import { G, Path, SVG } from '@wordpress/components';

export default (
<SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><Path fill="none" d="M0 0h24v24H0V0z" /><G><Path d="M7 11h2v2H7v-2zm14-5v14c0 1.1-.9 2-2 2H5c-1.11 0-2-.9-2-2l.01-14c0-1.1.88-2 1.99-2h1V2h2v2h8V2h2v2h1c1.1 0 2 .9 2 2zM5 8h14V6H5v2zm14 12V10H5v10h14zm-4-7h2v-2h-2v2zm-4 0h2v-2h-2v2z" /></G></SVG>
);
40 changes: 0 additions & 40 deletions packages/block-library/src/archives/index.js
View file
Open in desktop

This file was deleted.

32 changes: 1 addition & 31 deletions packages/block-library/src/archives/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*
* @return string Returns the post content with archives added.
*/
function render_block_core_archives( $attributes ) {
function gutenberg_render_block_core_archives( $attributes ) {
$show_post_count = ! empty( $attributes['showPostCounts'] );

$class = 'wp-block-archives';
Expand Down Expand Up @@ -115,33 +115,3 @@ function render_block_core_archives( $attributes ) {

return $block_content;
}

/**
* Register archives block.
*/
function register_block_core_archives() {
register_block_type(
'core/archives',
array(
'attributes' => array(
'align' => array(
'type' => 'string',
),
'className' => array(
'type' => 'string',
),
'displayAsDropdown' => array(
'type' => 'boolean',
'default' => false,
),
'showPostCounts' => array(
'type' => 'boolean',
'default' => false,
),
),
'render_callback' => 'render_block_core_archives',
)
);
}

add_action( 'init', 'register_block_core_archives' );
18 changes: 18 additions & 0 deletions packages/block-library/src/block/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"name": "core/block",
"title": "Reusable Block",
Copy link
Member

Choose a reason for hiding this comment

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

Have you thought about how translation will work? From #13693:

Localized properties are automatically wrapped in __ function calls on the backend and the frontend of WordPress. These translations are added as an inline script to the wp-block-library script handle.

https://github.com/WordPress/gutenberg/pull/13693/files#diff-4b5f9e280074f644def145cb4cc4fb6eR372

A challenge here is in knowing the textdomain to use if we were to call __ automatically when registering the block. I could see this being as simple as an additional textdomain property within the block manifest (which is maybe not great because it's not really inherent to a block). Or an argument when calling to register a block from a manifest file. Or some hacky wizardry with debug_backtrace + get_plugin_data to extract the Text Domain field from the plugin header.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is the biggest issue I identified so far. I still don't have a good idea of how this could work. textdomain as another attribute might be an option. I think it should be rather explicitly defined than depend on some traversal techniques. If this code would be placed in PHP, all translated string would have to provide this argument anyway.

I was also wondering if we can dynamically wrap with i18n functions when loading metadata. E.g.:

$settings = array(
	'title' => __( $metadata['title'], $metadata['textdomain'] ),
);

/cc @swissspidy

"category": "reusable",
"description": "Create content, and save it for you and other contributors to reuse across your site. Update the block, and the changes apply everywhere it’s used.",
"attributes": {
"ref": {
"type": "number"
}
},
"edit": "edit.js",
"supports": {
"customClassName": false,
"html": false,
"inserter": false
},
"renderCallback": "index.php"
}
35 changes: 0 additions & 35 deletions packages/block-library/src/block/index.js
View file
Open in desktop

This file was deleted.

15 changes: 1 addition & 14 deletions packages/block-library/src/block/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*
* @return string Rendered HTML of the referenced block.
*/
function render_block_core_block( $attributes ) {
function gutenberg_render_block_core_block( $attributes ) {
if ( empty( $attributes['ref'] ) ) {
return '';
}
Expand All @@ -28,16 +28,3 @@ function render_block_core_block( $attributes ) {

return do_blocks( $reusable_block->post_content );
}

register_block_type(
'core/block',
array(
'attributes' => array(
'ref' => array(
'type' => 'number',
),
),

'render_callback' => 'render_block_core_block',
)
);
29 changes: 29 additions & 0 deletions packages/block-library/src/calendar/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"name": "core/calendar",
"title": "Calendar",
"category": "widgets",
"icon": {
"slug": "calendar"
},
"description": "A calendar of your site’s posts.",
"keywords": [ "posts", "archive" ],
"attributes": {
"align": {
"type": "string"
},
"className": {
"type": "string"
},
"month": {
"type": "integer"
},
"year": {
"type": "integer"
}
},
"edit": "edit.js",
"supports": {
"align": true
},
"renderCallback": "index.php"
}
Loading