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

Scripts: Copy PHP files from src into build #38715

Merged
merged 2 commits into from Feb 11, 2022

Conversation

ryanwelcher
Copy link
Contributor

Description

When building dynamic blocks, there are many cases where splitting out the markup into an external PHP file is very helpful. The current build process copies the block.json file into the build directory but any .php files are not. This PR adds them to the copying process.

Testing Instructions

I created a block using npx @wordpress/create-block@ my-first-dynamic-block --template=@ryanwelcher/dynamic-block-template ( this contains an template.php file in the src directory )
Then ran node {FULLPATH}/gutenberg/packages/scripts/bin/wp-scripts.js -- build

Screenshots

Types of changes

Adds any .PHP files inside of src to it's corresponding location in build

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@ryanwelcher ryanwelcher added the [Package] Scripts /packages/scripts label Feb 10, 2022
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Feb 11, 2022
@gziolo gziolo changed the title Copy PHP files from src into build Scripts: Copy PHP files from src into build Feb 11, 2022
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Looks great. I added a changelog entry since people will suddenly start seeing PHP files in the build folder which might be surprising for them.

@gziolo gziolo merged commit 5b901cb into WordPress:trunk Feb 11, 2022
@gziolo gziolo added this to Done in Core JS Feb 11, 2022
@github-actions github-actions bot added this to the Gutenberg 12.7 milestone Feb 11, 2022
@ryanwelcher
Copy link
Contributor Author

Looks great. I added a changelog entry since people will suddenly start seeing PHP files in the build folder which might be surprising for them.

Thanks, @gziolo! I ALWAYS forget to add the changelog.

gziolo added a commit that referenced this pull request Feb 22, 2022
* Copy any .php files to the build folder.

* Add CHANGELOG entry

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
shimotmk added a commit to vektor-inc/vk-blocks-pro that referenced this pull request Feb 25, 2022
ブロックをロードするパスをsrcからbuildに変更
関連 #1034
WordPress/gutenberg#38715
@herndlm
Copy link

herndlm commented Feb 28, 2022

Hi, is there a way to disable this? I guess I need to override the webpack config?

@gziolo
Copy link
Member

gziolo commented Feb 28, 2022

Hi, is there a way to disable this? I guess I need to override the webpack config?

Yes, you would need to override the default config. Something like:

https://github.com/WordPress/gutenberg/tree/trunk/packages/scripts#extending-the-webpack-config

Can you explain the issue you have? It looks like the change introduced needs some tweaks so we are seeking more detailed feedback to provide better defaults.

@herndlm
Copy link

herndlm commented Mar 1, 2022

Sure, I'm happy to do so. We're using wordpress-scripts in some ouf WordPress PHP plugins where we also have JS parts. Maybe this is already a bit controversial. We have already some customization for that, but basically we have

  • some generic PHP classes in src/
  • a plugin-name.php in root, that adds e.g. a couple of shortcodes or bootstraps stuff that uses the classes from src/
  • a TypeScript app in assets/src, for that one we use wordpress-scripts for testing, linting and building/transpiling it to public from where we load / enqueue it in plugin-name.php with plain WP code

So basically, our use-case is using wordpress-scripts in a WordPress environment that is not Gutenberg :) And in that case I don't want the generic PHP classes end up in public.

@delawski
Copy link
Contributor

delawski commented Mar 2, 2022

We have already some customization for that, but basically we have

  • some generic PHP classes in src/
  • a plugin-name.php in root, that adds e.g. a couple of shortcodes or bootstraps stuff that uses the classes from src/
  • a TypeScript app in assets/src, for that one we use wordpress-scripts for testing, linting and building/transpiling it to public from where we load / enqueue it in plugin-name.php with plain WP code

We're using a similar directory structure in AMP WP plugin, i.e. the src directory stores PHP files while JS files are kept in assets/src and we're having analogical issues: ampproject/amp-wp#6947.

Right now the default source directory name of src is hardcoded in multiple places in the package (e.g. in the getWebpackEntryPoints() or in the webpack.config.js). I guess it'd be great to make it configurable in some way.

delawski added a commit to ampproject/amp-wp that referenced this pull request Mar 2, 2022
After introducing WordPress/gutenberg#38715, PHP files from the `src` folder are copied over to the output directory (i.e. `assets`). To prevent this, the `CopyWebpackPlugin` is excluded from the plugins list. It is safe since the only other file type the plugin copies is `block.json` that is not used in `amp-wp`.
@ryanwelcher
Copy link
Contributor Author

Follow up PR to move to an opt-in approach - #39171

@gziolo
Copy link
Member

gziolo commented Mar 3, 2022

Follow up PR to move to an opt-in approach - #39171

Landed, I will publish shortly to npm to remove friction as discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Scripts /packages/scripts [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Core JS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants