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

Fix #53806 #1555

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix #53806 #1555

wants to merge 4 commits into from

Conversation

costdev
Copy link
Contributor

@costdev costdev commented Aug 7, 2021

Providing register_block_type_from_metadata with the path to a custom named metadata file would search for path/to/metadata_file.json/block.json instead of correctly searching for path/to/metadata_file.json.

The following check is now carried out:

  • If the $file_or_folder path provided is a directory, search for $file_or_folder/block.json.
  • Otherwise, search for the $file_or_folder path exactly as provided.

Trac ticket: https://core.trac.wordpress.org/ticket/53806


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@gziolo
Copy link
Member

gziolo commented Sep 10, 2021

@costdev, thank you for looking at this issue. It was brought to my attention by @cr0ybot in the comment WordPress/gutenberg#25188 (comment). I'm trying to confirm whether the documentation is incorrect or the implementation. The comment from @coreyworrell in the Trac ticket might be valid:

Sorry, just realized it does work as long as the filename ends in block.json. I had previously just tried block-name.json which did not work. But something like whatever-name-block.json will work. Maybe the docs and comment could be updated to include that requirement if that is the intention.

@costdev
Copy link
Contributor Author

costdev commented Sep 10, 2021

Great @gziolo! If a discussion about the documentation follows, I'd suggest that we maybe shouldn't force third party block developers to use a file ending in ...block.json? We could just assume that ending if an alternative isn't explicitly provided - unless of course there's other code that currently relies on that ending in all cases.

@gziolo
Copy link
Member

gziolo commented Sep 10, 2021

I located the PR in Gutenberg that added the handling which allows passing a full path (ending with block.json): WordPress/gutenberg#22491. There is no discussion around that, it was driven by the convenience of providing a path to the file which was already computed in the code for core blocks 😄

Let me share some concerns I shared in another place regarding the name:

One thing that comes to my mind is integrations with the Plugin Directory or WP-CLI. A good example is the WP-CLI command to extract translations, it restricts processing to files that have block.json name:

https://github.com/wp-cli/i18n-command/blob/49fb6d98ebd5cbe85c87a5efc3c0877be9e358d2/src/MakePotCommand.php#L641-L642

I don't remember how the Plugin Directory detects if the plugin contains blocks, but for sure it would be simpler if the presence of the block.json file in the source code would be the way to do it.

Regarding Plugins Directory, I'm talking about the automated section that lists blocks shipped with the plugin:

Screen Shot 2021-09-10 at 09 50 53

Screen Shot 2021-09-10 at 09 52 06

Screen Shot 2021-09-10 at 09 52 27

@gziolo
Copy link
Member

gziolo commented Sep 10, 2021

@swissspidy, does the i18n command in WP-CLI detects only files named exactly block.json, or ending with block.json? What if we were to allow any name for block metadata files, could we teach WP-CLI command to discover them?

@tellyworth, a similar question related to Plugin Directory and Block Directory. How much does the name of the block metadata file impact how all accompanying tools work today? I'm mostly curious if the name block.json is mandatory or we can works with a wider set of options.

@swissspidy
Copy link
Member

@swissspidy, does the i18n command in WP-CLI detects only files named exactly block.json, or ending with block.json? What if we were to allow any name for block metadata files, could we teach WP-CLI command to discover them?

Right now it strictly looks for block.json, nothing else:

https://github.com/wp-cli/i18n-command/blob/01d6bd9c7933d67b87cc041620a4a5918914c633/src/MakePotCommand.php#L641-L642

You mentioned that line in your above comment yourself though 😄

We could change the WP-CLI command to scan any JSON file though and then check every file whether it's a block file. That might slow down things a bit if you have tons of other JSON files, but would be the most convenient option.

@gziolo
Copy link
Member

gziolo commented Nov 3, 2021

We need to make decision before WP 5.9 beta 1 release. It looks like updating the documentation makes the most sense here. We can always relax the requirement as proposed in the PR later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants