Style engine: move backend scripts to package #39736
Conversation
ramonjd
added
[Status] In Progress
|
Size Change: +1.15 kB (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
|
Hi, @ramonjd In case you missed this, @noisysocks recently merged best practices for Gutenberg PHP - https://github.com/WordPress/gutenberg/blob/trunk/lib/README.md. |
Thanks @Mamaduka I did read this. What it doesn't cover is this novel situation: How should we import individual PHP files – in this instance a class that might be used across Gutenberg – from There's a solution perhaps related to the way we import block I'm very keen to hear folks' advice on this, since I'm just hacking around. 😄 |
ramonjd
force-pushed
the
try/style-engine-block-supports-backend
branch
from
4ace927
to
bf9f444
Compare
ramonjd
force-pushed
the
update/style-engine-backend-to-package
branch
2 times, most recently
from
a7ab9e4
to
e7487ca
Compare
|
After hacking my way into a hole, I have the following open questions: What we want - high level
What we want - low level
OR Can we accept for now that the Style Engine PHP file lives as part of How?Not the way I've done it 😄 🤷
cc @youknowriad @apeatling @andrewserong and cc @gziolo and @ockham, who I know also have experience in the build process Thanks for your advice! |
|
Hi @ramonjd it's unclear to me from the message, what errors you had with our current way of doing things and why we need to change our approach? I'm all for using the best approach and don't mind changing but I'd like to understand the struggles with the current way we do things (block-library) which is the following:
I think the thing that differ from The above is mostly about moving this forward with what we already know/do. I think exploring alternatives is possible but it's unclear to me, what will we do here: Composer packages are interesting of course but we'd need to solve "auto loading" and whether it's fine for us to rely on composer auto-loading, we'd need to start a discussion in make/core with the whole community about it (I'm sure a lot of them will be delighted to use composer packages but I think there will be a resistance too). With composer packages, we won't solve the "duplication" problem though, if we load the package in core and Gutenberg, we'll have same class/file names. |
|
Thanks a lot for the explanation @youknowriad !
I'm not suggesting we change the approach. The source of my uncertainties stems mostly from not being sure whether what I'm doing is right at all 😄 I'm also conflicted in relation to how to deal with PHP unit tests: the file needs to be copied and renamed before the unit test runner kicks off (I've hacked this into bootstrap.php) I think you've confirmed the general direction, which is really helpful. I'm not too far off conceptually I believe, I just need to dig a little deeper as I'm still working out the parts.
Looks like this is where my initial attempt is probably not right. I'm writing to This branch actually works locally for me, and I (mostly) followed the block-library approach. The branch isn't building on CI yet though. I'll keep plugging away. Thanks again! 🙇 |
ramonjd
force-pushed
the
update/style-engine-backend-to-package
branch
2 times, most recently
from
86faf85
to
b94e602
Compare
|
I'm looking at the moment at |
|
@gziolo For the block supports, I think we should do the same as "block-library" (and the proposed changes for style engine here), meaning I believe we should just write them as if we were in Core, copy them automatically as part of the package update in core (like we do for blocks and parser) and rename them automatically in the gutenberg plugin in the web pack build. In previous version (5.9, 5.8) we kind of this that process manually: replace the files in core with the Gutenberg's files, rename the functions again. |
|
@youknowriad. It isn't that the syncing core blocks with WordPress core don't have any drawbacks. It was criticized several times in the past, including this issue #33241. Anyway, if you feel strongly about that approach then we can give it a try. From the perspective of someone merging the changes into WordPress core, it seems to be easier to leave everything to the script. |
I agree it's not perfect, especially for Gutenberg itself as it adds this "renaming stuff" complexity, but I think it still a bit better than having to do all these backports manually, don't you think?
Yes, that's the main selling point here. |
ramonjd
force-pushed
the
update/style-engine-backend-to-package
branch
2 times, most recently
from
c3b5173
to
05027cf
Compare
- moving Style Engine class and tests to packages - updating build script to copy package PHP into the build folder - rename methods and classes
ramonjd
force-pushed
the
update/style-engine-backend-to-package
branch
from
05027cf
to
1f4fab5
Compare
ramonjd
changed the title
|
This is looking good to me, do you think there's any blockers left? |
|
This all worked as advertised for me: ✅ Files as copied to /build folder and renamed and |
|
Thanks for testing @glendaviesnz !
I can't think of any, though I can't help but feel I have a limited overview of all the working parts. Things are working as they should so far so I suppose we could go ahead and merge and then 🤞 |
There was a problem hiding this comment.
Nice work @ramonjd, thanks for the follow-up! This is testing well for me:
✅ Builds file to correct location when running npm run build
✅ Builds file to correct location when running npm run dev
✅ Classes and function names correctly have gutenberg appended or prepended, respectively
✅ Tests are passing, and padding / margin behaviour still works correctly on the front-end
It looks like now that the test file is moved into the package, we should be able to remove the duplicate at https://github.com/wordpress/gutenberg/blob/44925a947fde29feaab512e0adf4291272209d59/phpunit/style-engine/class-wp-style-engine-gutenberg-test.php ?
There were good points raised in the discussion about what to do about the naming of the function calls / class names when the style engine is called from within the block supports files, but I reckon we could look at that separately if/when we want to improve things for the block supports directly.
This looks good to try to me!
|
LGTM to me as well. We'll need to make a specific patch for this for WordPress trunk (add the code to copied php files from packages) when we decide to include the style engine in Core. If it's for 6.0 that's easy (it's this week), otherwise it's going to be for 6.1 and we need to make sure we don't forget :P |
Good call. We've left it out of 6.0 because it's not really doing much. But I've set a reminder in my brain for 6.1 to get back to you 😄 |
Oh, yeah! I'll do that before merging this PR. Thanks for spotting that @andrewserong |
… in the package now.
💯 yes. |
Requires:
What?
A very naïve attempt at:
_gutenberg/_wprenaming is minimized for core patchesWhy?
Historical context/discussion:
How?
packages/style-engine_gutenberg)Testing Instructions
Build the project via
npm run devornpm run buildCheck that the PHP file in
packages/style-engineis copied to/buildand:*-gutenberg.php_GutenbergappendedFollow the test instructions in #39446, and check that the margin and padding block support styles are output correctly on the frontend.
Unit tests