Opened 5 years ago
Closed 5 years ago
#49196 closed enhancement (fixed)
Copy block metadata files for dynamic blocks
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch needs-testing |
Focuses: | javascript | Cc: |
Description
For dynamic blocks, there are several blocks whose metadata needs to be loaded in PHP. The attributes that are loaded via PHP are not exposed to the ReactNative components when Gutenberg loads the index.js files in the Mobile Apps. To support one master set of metadata that can be used to register in the PHP files and the Javascript files; I'd like to copy over the block.json files with their corresponding PHP files.
This would be a modification here:
https://github.com/WordPress/wordpress-develop/blob/14376be6e195ce9cc7f4bf237651504777b504fb/tools/webpack/packages.js#L97-L109
Suggested change dynamically lookup and block.json files that exist in the paths registered under phpfiles
. If the path has a defined block.json file then copy it to wp-includes/blocks/
and rename it to {block-name}.json
Attachments (5)
Change History (29)
This ticket was mentioned in PR #131 on WordPress/wordpress-develop by chipsnyder.
5 years ago
#1
5 years ago
#2
Thank you for opening this PR. It should be filed in WordPress trac:
https://core.trac.wordpress.org
This is only a mirror of svn repository and it's useful for testing against Continues Integration.
#3
@
5 years ago
Ooops, I see now that there is a track issue opened :) What a great integration with GitHub!
I left a few comments on GitHub, all of them related to coding styles. Other than that this patch looks good.
#5
@
5 years ago
At the moment all PHP files are located in one folder:
https://github.com/WordPress/wordpress-develop/tree/master/src/wp-includes/blocks
Given that this patch adds block.json
files as well, I think it would be easier to maintain everything if we put every block in its own subfolder.
chipsnyder commented on PR #131:
5 years ago
#6
Thanks for taking the time to look at it @gziolo. I updated the PR based on the suggestions and updated the diff in the ticket as well. I had failed to read the contributing guide close enough which is why I opened this PR first then added it to the ticket. I can close it after you get a chance to look at the changes :)
#7
@
5 years ago
@chipsnyder - I proposed a bit different approach where Webpack copies only PHP and JSON files from the whitelisted block folders. The patch you proposed didn't quite work because of missing block.json
files for all the listed packages. Even for the upcoming new version of packages, only Shortcode block will contain block.json
file.
5 years ago
#8
Can you have a look at https://core.trac.wordpress.org/attachment/ticket/49196/49196-block-folders.diff which I prepared to make it work without errors when running npm run build:dev
in WordPress core?
If you are fine with the changes proposed, it would be great to validate them on Travis in this branch.
#9
@
5 years ago
@gziolo I like the approach here better. I think it makes the references more clear by moving the blocks to their respective folders.
If we go with this approach, we'll need to make a few changes back in Gutenberg as well.
1) Update the copy of the PHP files to be in nested folders https://github.com/WordPress/gutenberg/blob/master/webpack.config.js#L120-L122
2) Update the copy of the JSON files to be in nested folders https://github.com/WordPress/gutenberg/blob/master/webpack.config.js#L155-L157 (Or consolidate with step 1)
3) Update the JSON reference in shortcode https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/shortcode/index.php#L24
4) Update the references to the PHP files. This is the step I wasn't sure about before and what had me hesitant about taking in on as part of the shortcode PR. I think that this is the only place we would need to make that update though https://github.com/WordPress/gutenberg/blob/eeb13f45a4f890179cfc9b0203f4019eb4d19f41/lib/blocks.php#L41-L62
5) Update the build script for zipping plugins https://github.com/WordPress/gutenberg/blob/master/bin/build-plugin-zip.sh#L112
These are probably pretty straight forward changes now that I'm learning more about how the project organized. There might be another step in there that I'm not aware of yet. I'm happy to work with you to get those done.
#10
@
5 years ago
@chipsnyder - great feedback. I was in the middle of the patch when you commented. It was very useful to do the sanity check with your list. It's ready here: https://github.com/WordPress/gutenberg/pull/19786.
#11
@
5 years ago
@gziolo With the changes you made to the Gutenberg repo I think these look good now. Feel free to push to branch I have open for Travis to run. If you would like me to apply the diff that's fine too.
@
5 years ago
adding the Updated diff from the changes in GitHub. This Diff fixes the build issues with the last diff
#13
follow-up:
↓ 14
@
5 years ago
- Keywords 2nd-opinion needs-testing added
Quick look at 49196-block-folders_updated.diff: (Mass) renaming/moving of existing PHP files seems very risky. Is there a chance that a plugin may be doing something like require_once( ABSPATH . WPINC . '/blocks/latest-posts.php' );
and moving that file would "bring the site down" with a PHP fatal error? Is it possible to avoid the moving and renaming?
This concerns mostly the "build tools" as far as I understand. Don't think it is a good idea to change existing files (in production) to accommodate changes to how the building is done. Rather the build process should be accommodating the existing files.
#14
in reply to:
↑ 13
;
follow-up:
↓ 15
@
5 years ago
Replying to azaozz:
Quick look at 49196-block-folders_updated.diff: (Mass) renaming/moving of existing PHP files seems very risky. Is there a chance that a plugin may be doing something like
require_once( ABSPATH . WPINC . '/blocks/latest-posts.php' );
and moving that file would "bring the site down" with a PHP fatal error? Is it possible to avoid the moving and renaming?
I wasn't aware that the file location might be treated as a contract. If this is the case then we can consider keeping the current names and handle the rest differently.
Another option would be to proxy old locations to the new one.
#15
in reply to:
↑ 14
@
5 years ago
Replying to gziolo:
I wasn't aware that the file location might be treated as a contract. If this is the case then we can consider keeping the current names and handle the rest differently.
Yeah, for better or for worse pretty much all files in WP are not "source" files, they are "production" files. "Architectural changes" like moving and/or renaming them are usually not a good idea :)
That's the reason why all the old js is moved back to the previous places when building, from /scr/js
to /build/wp-admin/js
and /build/wp-includes/js
Another option would be to proxy old locations to the new one.
Yes, that's possible. Would probably need to keep the existing files but then delete all the code there and just add include [path to the moved/renamed file]
. However thinking this is a "last resort" solution for when a file has to be moved and there's absolutely no other way to do it.
#16
@
5 years ago
With the issue around changing directories in mind, the diff "49196_updated_Jan15.diff" will take care of the required changes without the need to relocate files.
@gziolo, would you like me to update the PR with these changes?
#17
@
5 years ago
I uploaded another proposal which goes along with https://github.com/WordPress/gutenberg/pull/19786/commits/645579df583498cf3482ffc2d18daaf2f7c5b7b2 :)
I tested locally with fake block.json
file added in node_modules/@wordpress/block-library/src/categories/
folder.
This ticket was mentioned in PR #143 on WordPress/wordpress-develop by chipsnyder.
5 years ago
#18
chipsnyder commented on PR #143:
5 years ago
#19
@gziolo Here's the new PR with the adjusted path
chipsnyder commented on PR #131:
5 years ago
#20
Reopening with new changes
#22
follow-up:
↓ 23
@
5 years ago
It passes all tests on Travis, works properly in my testing. It won't copy any block.json
file until the next npm release where Shortcodes block introduces it.
@azaozz - can we proceed? You added 2nd-opinion
flag to this patch, so I guess I should wait for your response or yet another reviewer.
patch for issue 49196