Script Modules API: update the code and move it to the compat/wordpress-6.5 folder #57778

cbravobernal · 2024-01-11T22:38:24Z

What?

Modules API has been approved in Core. But was as experimental instead of 6.5 compat folder.

Script Modules API wordpress-develop#5818

github-actions · 2024-01-11T22:39:34Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/compat/wordpress-6.5/class-wp-script-modules.php
❔ lib/compat/wordpress-6.5/scripts-modules.php
❔ lib/experimental/script-modules.php
❔ lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php
❔ lib/experimental/interactivity-api/modules.php
❔ lib/load.php

lib/compat/wordpress-6.5/class-wp-script-modules.php

lib/compat/wordpress-6.5/scripts-modules.php

luisherranz · 2024-01-12T09:23:05Z

Let's wait until a final decision on the naming has been made to merge this.

cbravobernal · 2024-01-12T09:24:11Z

Let's wait until a final decision on the naming has been made to merge this.

I was about to say the same.

cbravobernal · 2024-01-12T10:43:19Z

~~The PR still needs some updates in tests.~~ Done

luisherranz

We still need to keep gutenberg_register_module, gutenberg_enqueue_module and gutenberg_dequeue_module for at least a couple of versions in Gutenberg, in case someone was already using it.

I don't know how to do deprecation notices in WordPress, but if that is possible, we should add them.

cbravobernal · 2024-01-12T16:33:26Z

We still need to keep gutenberg_register_module, gutenberg_enqueue_module and gutenberg_dequeue_module for at least a couple of versions in Gutenberg, in case someone was already using it.

I don't know how to do deprecation notices in WordPress, but if that is possible, we should add them.

This could be an example about how to do it. I'll update it.

gutenberg/lib/block-supports/duotone.php

Line 188 in c454a02

_deprecated_function( __FUNCTION__, '6.3.0' );

Works like a charm!

sirreal · 2024-01-23T17:32:40Z

and I cannot reproduce it locally.

Any ideas?

I was able to reproduce locally using wp-env with this PR checked out and using latest wordpress-develop for core and using PHP 7.0. This did not have any problem for me using the default PHP version.

I had a .wp-env.override.json like this:

{
	"core": "../../wordpress-develop/build",
	"phpVersion": "7.0"
}

I pushed a fix, feel free to do what you like with it. I suspect that in PHP 7 it's parsing the entire file so it sees the class name even though it looks like there's an early return.

luisherranz · 2024-01-24T09:03:59Z

Thanks @sirreal!

I suspect that in PHP 7 it's parsing the entire file so it sees the class name even though it looks like there's an early return.

Any clue why returning early was not working on this file, but it seems to work fine in other files?

if ( class_exists( 'WP_Script_Modules' ) ) {
	return;
}

luisherranz

Ok, this should be ready to go, but the e2e tests are failing because this hasn't been committed to WordPress Core yet:

Script Modules API: Move the script modules to the footer in classic themes wordpress-develop#5931

We have to wait until that happens to be able to merge.

luisherranz · 2024-01-24T11:40:48Z

Now that WordPress/wordpress-develop#5931 has been committed to Core (thanks @youknowriad!) the tests pass and we can merge this.

@c4rl0sbr4v0 can you take a final look before I merge?

sirreal · 2024-01-24T12:31:48Z

Thanks @sirreal!

Any clue why returning early was not working on this file, but it seems to work fine in other files?

Not really and that worries me 😅 I'd like to do some more experimentation but can't invest the time right now.

All versions of PHP seem to see the class in the same file when there's an early return. The safest thing would be to avoid that pattern completely. See this example:

<?php
if ( class_exists( 'A' ) ) {
	echo "EXISTS A\n";
	return;
} else {
	echo "NO EXISTS A\n";
}
class A {}

This prints "EXISTS A" in every PHP version. I'm not sure why the "early return" ever works without errors.

From the include docs:

The documentation below also applies to require.
…
If there are functions defined in the included file, they can be used in the main file independent if they are before return or after. If the file is included twice, PHP will raise a fatal error because the functions were already declared. It is recommended to use include_once instead of checking if the file was already included and conditionally return inside the included file.

That mentions functions but I assume that also applies to classes, but 🤷 it's not very clear.

cbravobernal · 2024-01-24T12:48:34Z

I removed some "we" in the docs, as is a common review in WordPress Core. Nothing special, apart from that, it's OK to merge this once tests are done.

cbravobernal · 2024-01-24T12:50:35Z

Any clue why returning early was not working on this file, but it seems to work fine in other files?

I've seen in other parts of the project that contributors add that bail out in both class-wp-script-modules.php and load.php file.

luisherranz · 2024-01-24T12:52:49Z

I'm not sure why the "early return" ever works without errors.

Yep. I still see it working in other places just fine 🙂

Something must be going on, but 🤷‍♂️

sirreal · 2024-01-24T12:55:33Z

That mentions functions but I assume that also applies to classes, but 🤷 it's not very clear.

Nope, it seems like it's just functions that are problematic. I'm really not sure what's going on with that one case in certain PHP versions.

Because they don't seem to work: #57778 (comment)

@access

* Initial SDP version * Rename `initialState` to `state` * Fix incorrect package * Properly remove the SDP during the e2e tests * Update multiline comments to fit WP standards * Add @access private to WP_Interactivity_API_Directives_Processor * Remove array check and rename bookmarks in WP_Interactivity_API_Directives_Processor * Dont print config/state if it's empty * Add failing test for </ > * Add missing `object` word * Missing indentation on ternary conditionals * Improve namespace extraction from directive value * Move modified properties to the end in set_style_property * Replace tabindex comment * Remove filter after processing and bump priority * Replace own is_void with HTML API one * Missing ternary indentation * Avoid early returns Because they don't seem to work: #57778 (comment) * Update wp_register_script_module function * Replaces some incorrect docblock comments with block comments * Fix indent ternaries (again) * Add more info about how extract_directive_value behaves * Add more edge cases * Improve wp_interactivity_process_directives_of_interactive_blocks comment --------- Co-authored-by: Carlos Bravo <[email protected]>

youknowriad · 2024-01-25T09:34:04Z

packages/block-library/src/file/index.php

-	}
+	wp_register_script_module(
+		'@wordpress/block-library/file-block',
+		gutenberg_url( '/build/interactivity/file.min.js' ),


Hey folks, this is breaking the package update in Core. The block library php files are copied as is into WordPress Core. Meaning we should ensure they work there without changes.

These Gutenberg specific function calls are there for not something we should be using. I see that the previous versions had these being the IS_GUTENBERG_PLUGIN to solve that.

Opsss. Our bad. I'll open a PR right away!

Do you want us to release a 17.6.1 RC with the fix?

cc: @c4rl0sbr4v0

We will do a RC2. We don't need a 17.6.1 yet, as 17.6 won't be released until next week 😅

There you go:

Prevent usage of gutenberg_url in block-library #58242

getdave · 2024-01-26T11:46:52Z

I noticed this PR was merged after I raised the PHP Sync Tracking Issue for WP 6.5 and has changed PHP files that may need backporting to WP Core.

Please forgive the ping, but I marked as Needs PHP backport and also added to the tracking Issue.

luisherranz · 2024-01-26T11:51:12Z

This has already been backported in all the PRs related to the Script Modules API. (I reported it yesterday here but I forgot to add the backport label here, sorry 🙏).

luisherranz · 2024-01-26T11:52:07Z

I removed the Needs PHP backport label and added the Backported to WP Core one. @getdave let me know if that's fine.

sirreal · 2024-02-06T12:10:41Z

There's an issue about the class_exists errors: #58467 (comment)

getdave · 2024-02-06T19:12:04Z

Thank you for the detailed updates @luisherranz

cbravobernal added [Type] Code Quality Issues or PRs that relate to code quality [Feature] Script Modules API Related to the Script Modules API that adds support for native ES modules and import maps labels Jan 11, 2024

cbravobernal self-assigned this Jan 11, 2024

cbravobernal commented Jan 11, 2024

View reviewed changes

lib/compat/wordpress-6.5/class-wp-script-modules.php Outdated Show resolved Hide resolved

luisherranz reviewed Jan 12, 2024

View reviewed changes

lib/compat/wordpress-6.5/scripts-modules.php Outdated Show resolved Hide resolved

luisherranz reviewed Jan 12, 2024

View reviewed changes

lib/compat/wordpress-6.5/scripts-modules.php Outdated Show resolved Hide resolved

luisherranz mentioned this pull request Jan 12, 2024

Script Modules API WordPress/wordpress-develop#5818

Closed

cbravobernal force-pushed the update/move-modules-api-to-6-5 branch from a5017bc to c850f71 Compare January 12, 2024 14:24

luisherranz requested changes Jan 12, 2024

View reviewed changes

cbravobernal added 5 commits January 12, 2024 17:24

Move modules api to 6.5 folder

0bade94

Update functions to use wp_modules instead of gutenberg_modules

046d43e

Prevent class redeclaration

3692916

Add classic themes conditional

737a227

Yet another script position movement

c454a02

cbravobernal force-pushed the update/move-modules-api-to-6-5 branch from c850f71 to c454a02 Compare January 12, 2024 16:24

Add gutenberg_*_module deprecations

6ff5ddf

cbravobernal marked this pull request as ready for review January 12, 2024 17:16

cbravobernal requested review from spacedmonkey, ntwb, nerrad, ajitbohra, artemiomorales, michalczaplinski and tellthemachines as code owners January 12, 2024 17:16

cbravobernal changed the title ~~Modules API: (WIP) - Move modules api to 6.5 folder~~ Modules API: Move modules api to 6.5 folder Jan 12, 2024

luisherranz approved these changes Jan 24, 2024

View reviewed changes

luisherranz marked this pull request as ready for review January 24, 2024 09:34

luisherranz requested a review from gziolo as a code owner January 24, 2024 09:34

Remove we from docs

16150b8

cbravobernal enabled auto-merge (squash) January 24, 2024 12:51

cbravobernal merged commit 01fb505 into trunk Jan 24, 2024
55 checks passed

cbravobernal deleted the update/move-modules-api-to-6-5 branch January 24, 2024 13:17

github-actions bot added this to the Gutenberg 17.6 milestone Jan 24, 2024

luisherranz added a commit that referenced this pull request Jan 24, 2024

Avoid early returns

fab5662

Because they don't seem to work: #57778 (comment)

youknowriad reviewed Jan 25, 2024

View reviewed changes

luisherranz mentioned this pull request Jan 25, 2024

Prevent usage of gutenberg_url in block-library #58242

Merged

cbravobernal mentioned this pull request Jan 25, 2024

Script Modules API: Add import map polyfill #58263

Merged

getdave added the Needs PHP backport Needs PHP backport to Core label Jan 26, 2024

luisherranz added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Jan 26, 2024

sirreal mentioned this pull request Feb 6, 2024

Fatal error: PHP class_exists() bail out at top of class files is not robust across PHP versions. #58467

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script Modules API: update the code and move it to the compat/wordpress-6.5 folder #57778

Script Modules API: update the code and move it to the compat/wordpress-6.5 folder #57778

cbravobernal commented Jan 11, 2024 •

edited by luisherranz

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading

luisherranz commented Jan 12, 2024

cbravobernal commented Jan 12, 2024

cbravobernal commented Jan 12, 2024 •

edited

Loading

luisherranz left a comment

cbravobernal commented Jan 12, 2024 •

edited

Loading

sirreal commented Jan 23, 2024

luisherranz commented Jan 24, 2024

luisherranz left a comment

luisherranz commented Jan 24, 2024

sirreal commented Jan 24, 2024 •

edited

Loading

cbravobernal commented Jan 24, 2024

cbravobernal commented Jan 24, 2024

luisherranz commented Jan 24, 2024

sirreal commented Jan 24, 2024

youknowriad Jan 25, 2024

luisherranz Jan 25, 2024

luisherranz Jan 25, 2024

cbravobernal Jan 25, 2024

luisherranz Jan 25, 2024

getdave commented Jan 26, 2024

luisherranz commented Jan 26, 2024

luisherranz commented Jan 26, 2024

sirreal commented Feb 6, 2024

getdave commented Feb 6, 2024

Script Modules API: update the code and move it to the compat/wordpress-6.5 folder #57778

Script Modules API: update the code and move it to the compat/wordpress-6.5 folder #57778

Conversation

cbravobernal commented Jan 11, 2024 • edited by luisherranz Loading

What?

github-actions bot commented Jan 11, 2024 • edited Loading

luisherranz commented Jan 12, 2024

cbravobernal commented Jan 12, 2024

cbravobernal commented Jan 12, 2024 • edited Loading

luisherranz left a comment

Choose a reason for hiding this comment

cbravobernal commented Jan 12, 2024 • edited Loading

sirreal commented Jan 23, 2024

luisherranz commented Jan 24, 2024

luisherranz left a comment

Choose a reason for hiding this comment

luisherranz commented Jan 24, 2024

sirreal commented Jan 24, 2024 • edited Loading

cbravobernal commented Jan 24, 2024

cbravobernal commented Jan 24, 2024

luisherranz commented Jan 24, 2024

sirreal commented Jan 24, 2024

youknowriad Jan 25, 2024

Choose a reason for hiding this comment

luisherranz Jan 25, 2024

Choose a reason for hiding this comment

luisherranz Jan 25, 2024

Choose a reason for hiding this comment

cbravobernal Jan 25, 2024

Choose a reason for hiding this comment

luisherranz Jan 25, 2024

Choose a reason for hiding this comment

getdave commented Jan 26, 2024

luisherranz commented Jan 26, 2024

luisherranz commented Jan 26, 2024

sirreal commented Feb 6, 2024

getdave commented Feb 6, 2024

cbravobernal commented Jan 11, 2024 •

edited by luisherranz

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading

cbravobernal commented Jan 12, 2024 •

edited

Loading

cbravobernal commented Jan 12, 2024 •

edited

Loading

sirreal commented Jan 24, 2024 •

edited

Loading