Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#49196 closed enhancement (fixed)

Copy block metadata files for dynamic blocks

Reported by: chipsnyder's profile chipsnyder Owned by: gziolo's profile gziolo
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)

49196.diff (1.2 KB) - added by chipsnyder 5 years ago.
patch for issue 49196
49196_updated_Jan15.diff (1.2 KB) - added by chipsnyder 5 years ago.
Updated Diff based on the code review in Github
49196-block-folders.diff (72.3 KB) - added by gziolo 5 years ago.
Folder per the block file
49196-block-folders_updated.diff (6.1 KB) - added by chipsnyder 5 years ago.
adding the Updated diff from the changes in GitHub. This Diff fixes the build issues with the last diff
49196-block-json-folder.diff (2.1 KB) - added by gziolo 5 years ago.

Download all attachments as: .zip

Change History (29)

This ticket was mentioned in PR #131 on WordPress/wordpress-develop by chipsnyder.


5 years ago
#1

@chipsnyder
5 years ago

patch for issue 49196

gziolo commented on PR #131:


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 @gziolo
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.

#4 @gziolo
5 years ago

  • Focuses javascript added
  • Milestone changed from Awaiting Review to 5.4

#5 @gziolo
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
5 years ago

Updated Diff based on the code review in Github

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 :)

@gziolo
5 years ago

Folder per the block file

#7 @gziolo
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.

gziolo commented on PR #131:


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 @chipsnyder
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 @gziolo
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 @chipsnyder
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.

#12 @chipsnyder
5 years ago

PR is updated now.

@chipsnyder
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: @azaozz
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.

Last edited 5 years ago by azaozz (previous) (diff)

#14 in reply to: ↑ 13 ; follow-up: @gziolo
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 @azaozz
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.

Last edited 5 years ago by azaozz (previous) (diff)

#16 @chipsnyder
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 @gziolo
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

#21 @chipsnyder
5 years ago

Updated the original PR with the changes

#22 follow-up: @gziolo
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.

#23 in reply to: ↑ 22 @azaozz
5 years ago

  • Keywords 2nd-opinion removed

Replying to gziolo:

can we proceed? You added 2nd-opinion flag...

Sure, looks good.

Yeah, the 2nd-opinion generally means "needs more eyes/reviews" but also "I may be wrong/overthinking it, ideally another person would look at it" :)

#24 @gziolo
5 years ago

  • Owner set to gziolo
  • Resolution set to fixed
  • Status changed from new to closed

In 47168:

Build: Copy block metadata files for dynamic blocks.

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; with this patch, the block.json files are copied over with their corresponding PHP files.

Props chipsnyder and azaozz.

Fixes #49196.

Note: See TracTickets for help on using tickets.