Opened 6 months ago
Closed 5 months ago
#60862 closed defect (bug) (fixed)
wp_localize_script() on login_enqueue_scripts hook change in behavior
Reported by: | salcode | Owned by: | swissspidy |
---|---|---|---|
Milestone: | 6.5.3 | Priority: | normal |
Severity: | normal | Version: | 6.5 |
Component: | Script Loader | Keywords: | has-patch commit fixed-major dev-reviewed |
Focuses: | Cc: |
Description
I'm seeing this change in behavior going from 6.4.3
to 6.5-RC4
.
When I add the following code snippet to a file in mu-plugins
.
<?php add_action( 'login_enqueue_scripts', function() { wp_localize_script( 'wp-util', 'salcodeExample', [ 'answerToTheUltimateQuestionOfLifeTheUniverseAndEverything' => 42, ] ); } );
and then I go to /wp-login.php
and View Source.
6.4.3 and below
I see the line
var salcodeExample = {"answerToTheUltimateQuestionOfLifeTheUniverseAndEverything":"42"};
inside the <script type="text/javascript" id="wp-util-js-extra">
block.
This is the expected behavior.
6.5-RC4
The <script type="text/javascript" id="wp-util-js-extra">
block is still included but the line we added via our code does NOT appear.
Notes:
- I've tested with both the
wp-util
and theuser-profile
hooks and get the same behavior. - I'm only seeing this behavior when calling
wp_localize_script()
on thelogin_enqueue_scripts
(specifically, I'm not seeing this behavior when using thewp_enqueue_scripts
hook) - This issue was originally identified in this GitHub issue https://github.com/salcode/wp-fast-login/issues/26
Change History (27)
This ticket was mentioned in PR #6334 on WordPress/wordpress-develop by @aslamdoctor.
6 months ago
#1
- Keywords has-patch added
#2
@
6 months ago
- Milestone changed from Awaiting Review to 6.5
Using git bisect and the example code, I found that this was introduced in [57377].
Moving to 6.5 for visibility but this might be something that needs to be fixed in 6.5.1
#3
@
6 months ago
If I learned something from [57286] / #58696 it's that block registration causes scripts to be initialized and localized very early, which can lead to some odd side effects (as that ticket demonstrates). So this could be a starting point for debugging.
The attached PR fixes the symptom but not the underlying bug. But we should definitely identify and address the root cause here if possible. Plus, add test coverage.
#4
@
6 months ago
So it's definitely because of a change with the block registration. Previously, many blocks had editorScript
and viewScript
entries. That caused scripts to be registered very early on init, leading to $wp_scripts
being defined early. In [57377] those were all reworked to use the Interactivity API instead, so by default no early script registration happens.
The result is simple: when login_enqueue_scripts
fires, no scripts have been registered yet. That only happens implicitly with this wp_enqueue_script( 'user-profile' );
call here which happens right _after_ the login_header()
call that fires login_enqueue_scripts
: https://github.com/WordPress/wordpress-develop/blob/f1c098e77f67ae2469e519757c7c3829288da5d6/src/wp-login.php#L1494
In other words, the code used to work mostly by pure coincidence. On any other page it's actually very similar, $wp_scripts
isn't really initialized in a deterministic way. For example on the homepage it usually happens in _wp_admin_bar_init()
.
That's also why I wasn't able to reproduce this issue at first. If some other plugin happens to register a script on the login page early on, then you won't notice any issues.
That also means this issue might not be too widespread.
It's just that by default wp_scripts()
is now not called early on the login page anymore.
Long story short:
This bug actually existed since forever, it just so happens that early block registration was hiding this bug.
A change similar to the one in the PR would fix the issue. IMHO that can be done in 6.5.1 or even 6.6 though.
Longer term, initializing $wp_scripts
in a more deterministic way would probably be better. On the other hand, wp_scripts()
seems to be designed to be used like this, so yeah maybe that's not needed.
#5
@
6 months ago
- Keywords needs-unit-tests added
- Milestone changed from 6.5 to 6.5.1
Thanks @swissspidy!
While this might not have been something that was intended, it is a behavior change from 6.4 to 6.5 that I think is worth fixing, so I'm milestoning it for 6.5.1 but agree that it needs an automated test. Maybe An E2E to ensure this localization on login_enqueue_scripts
works as expected.
As a workaround, calling wp_scripts();
ensures that the global is setup.
<?php add_action( 'login_enqueue_scripts', function() { wp_scripts(); wp_localize_script( 'wp-util', 'salcodeExample', [ 'answerToTheUltimateQuestionOfLifeTheUniverseAndEverything' => 42, ] ); } );
This ticket was mentioned in Slack in #core by jorbin. View the logs.
6 months ago
#7
@
6 months ago
I have a similar issue where wp_localize_script used to work in a block registration, but since WP 6.5 that localization isn't happening at time where the data is usable by the JS. Because I can easily get that data in another way I have just worked around it. But I'm curious if this is related or not.
function moneygauge_cta_block_init() { $script_handle = 'moneygauge-cta-block-editor'; wp_register_script( $script_handle, plugins_url('build/index.js', __FILE__), array('wp-blocks', 'wp-editor'), filemtime(plugin_dir_path(__FILE__) . 'build/index.js') ); // Assuming your image is located at <your-plugin-dir>/assets/handsup.jpg $image_url = plugins_url('assets/handsup.jpeg', __FILE__); wp_localize_script($script_handle, 'moneygaugeData', [ 'imageUrl' => $image_url, // This is the full URL to the image ]); register_block_type(__DIR__ . '/build', [ 'editor_script' => $script_handle, ]); } add_action('init', 'moneygauge_cta_block_init');
#8
@
6 months ago
@GuerillaDesigns I don't think that's related.
If you pass editor_script
to register_block_type
, it will automatically call register_block_script_handle
for it, which means it will call wp_register_script
. But you are already registering the script yourself, so that will get overridden.
If you're seeing different behavior in 6.5, that might be because of [57590]. I recommend opening a new ticket for this. FYI @gziolo.
#10
@
6 months ago
If you're seeing different behavior in 6.5, that might be because of [57590]. I recommend opening a new ticket for this. FYI @gziolo.
There are no functional changes in the linked changeset that would impact registration. It's only a special case that allows the script handle to be provided in the asset file. So it might be something else.
If you pass editor_script to register_block_type, it will automatically call register_block_script_handle for it, which means it will call wp_register_script. But you are already registering the script yourself, so that will get overridden.
If the script handle gets passed then during the block registration it only gets linked to the block, it never get registrated again. The registration only happens when the path to the file gets provided in block.json
.
This ticket was mentioned in Slack in #core by jorbin. View the logs.
6 months ago
This ticket was mentioned in Slack in #core by jorbin. View the logs.
5 months ago
#15
@
5 months ago
@aslamdoctor, thanks for the initial patch. Are you able to add an automated test to it to help ensure this bug never comes back?
#17
@
5 months ago
@aslamdoctor thanks, but this test isn't actually testing the code due to how globals work. If you run the test in isolation without the change, it fails as expected. However if you run it with the entire suite, it will be a false positive.
I've added an e2e test in https://github.com/WordPress/wordpress-develop/pull/6334/commits/8ecd3e01fdfbfc1c87ebcc6b254b620f147ffa25 what do you think?
#18
@
5 months ago
@jorbin just checked the e2e test and it seems to be working as expected.
few points:
- Do we need the console.log() on this line ?
- Now, as this e2e test is the proper way to test this patch, should we remove the PHPUnit test?
- I noticed there are some errors on E2E test on github repo which are related to file not found at /mu-plugins/login.php file. Should we be concerned about this?
#19
@
5 months ago
- No
- Yes/No. The PHPUnit test could also be improved so it actually works.
- Yes, it means the test is failing right now.
This ticket was mentioned in Slack in #core by jorbin. View the logs.
5 months ago
#21
@
5 months ago
- Keywords commit added
I removed the PHP test but I agree with @swissspidy that it could comeback if it actually works.
I fixed up the test so that it creates the mu-plugins folder. I also adjusted it so that it works when running the tests vs both src and build.
Planning to commit later today unless there is an objection.
#22
@
5 months ago
Thanks @jorbin
Regarding PHP test, if you restore the php unit test file and then do the test by restoring the src/wp-includes/functions.wp-scripts.php
file to it's previous state, the assertion returns false because it finds the $wp_scripts
is not defined. But with the patch, the assertion returns true. So yes the unit test works properly.
#23
@
5 months ago
@aslamdoctor If you restore src/wp-includes/functions.wp-scripts.php
and run the entire test suite (not just the individual test), the test also passes due to side effects.
@swissspidy asked me for some time to review when it's not late night, so holding off on committing for the time being.
#24
@
5 months ago
- Owner set to swissspidy
- Resolution set to fixed
- Status changed from new to closed
In 58068:
@swissspidy commented on PR #6334:
5 months ago
#25
Committed in https://core.trac.wordpress.org/changeset/58068
Global variable
$wp_scripts
is not defined at the moment when it is being used bywp_localize_scripts()
hook on login page. So to define it properly, there is a functionwp_scripts()
which checks and creates a new object ofWP_Scripts
class if it is not defined.Trac ticket: https://core.trac.wordpress.org/ticket/60862