Make WordPress Core

Opened 4 months ago

Closed 2 months ago

#60862 closed defect (bug) (fixed)

wp_localize_script() on login_enqueue_scripts hook change in behavior

Reported by: salcode's profile salcode Owned by: swissspidy's profile 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:

  1. I've tested with both the wp-util and the user-profile hooks and get the same behavior.
  2. I'm only seeing this behavior when calling wp_localize_script() on the login_enqueue_scripts (specifically, I'm not seeing this behavior when using the wp_enqueue_scripts hook)
  3. 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.


4 months ago
#1

  • Keywords has-patch added

Global variable $wp_scripts is not defined at the moment when it is being used by wp_localize_scripts() hook on login page. So to define it properly, there is a function wp_scripts() which checks and creates a new object of WP_Scripts class if it is not defined.

Trac ticket: https://core.trac.wordpress.org/ticket/60862

#2 @jorbin
4 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 @swissspidy
4 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 @swissspidy
4 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 @jorbin
3 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.


3 months ago

#7 @GuerillaDesigns
3 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 @swissspidy
3 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.

#9 @swissspidy
3 months ago

  • Component changed from Login and Registration to Script Loader

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

#11 @davidbaumwald
3 months ago

  • Milestone changed from 6.5.1 to 6.5.2

Milestone renamed

#12 @jorbin
3 months ago

  • Milestone changed from 6.5.2 to 6.5.3

This ticket was mentioned in Slack in #core by jorbin. View the logs.


3 months ago

This ticket was mentioned in Slack in #core by jorbin. View the logs.


3 months ago

#15 @jorbin
3 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?

#16 @aslamdoctor
3 months ago

  • Keywords needs-unit-tests removed

@jorbin test has been added now.

#17 @jorbin
3 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 @aslamdoctor
3 months ago

@jorbin just checked the e2e test and it seems to be working as expected.

few points:

  1. Do we need the console.log() on this line ?

https://github.com/WordPress/wordpress-develop/pull/6334/files#diff-ac85ccb848d1c1b4eaa345b6e66b79d32edfa6ad891e91448b4ce3b09b043671R16

  1. Now, as this e2e test is the proper way to test this patch, should we remove the PHPUnit test?
  1. 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 @swissspidy
2 months ago

  1. No
  2. Yes/No. The PHPUnit test could also be improved so it actually works.
  3. Yes, it means the test is failing right now.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


2 months ago

#21 @jorbin
2 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 @aslamdoctor
2 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 @jorbin
2 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 @swissspidy
2 months ago

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

In 58068:

Script Loader: Ensure wp_localize_script() works when called early.

Before, wp_localize_script() did not work when the $wp_scripts global was not already set (for example because of a script registration happening elsewhere) and even emitted a warning in that case. Due to side effects such as block registration early in the load process, this usually never happened. However, the absence of these side effects in 6.5 caused the wp_localize_script() to no longer work in places such as the login_enqueue_scripts.

By calling wp_scripts() in wp_localize_script(), the $wp_scripts global is automatically set if needed, restoring previous behavior. Adds both a PHP unit test and an e2e test to verify this use case. Hat tip: jorbin.

Happy birthday, Aaron!

Props salcode, aslamdoctor, jorbin, swissspidy.
Fixes #60862.

#26 @jorbin
2 months ago

  • Keywords fixed-major dev-reviewed added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopeneing to backport for 6.5.3

#27 @jorbin
2 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 58078:

Script Loader: Ensure wp_localize_script() works when called early.

Before, wp_localize_script() did not work when the $wp_scripts global was not already set (for example because of a script registration happening elsewhere) and even emitted a warning in that case. Due to side effects such as block registration early in the load process, this usually never happened. However, the absence of these side effects in 6.5 caused the wp_localize_script() to no longer work in places such as the login_enqueue_scripts.

By calling wp_scripts() in wp_localize_script(), the $wp_scripts global is automatically set if needed, restoring previous behavior. Adds both a PHP unit test and an e2e test to verify this use case. Hat tip: jorbin.

Thanks for the birthday wishes, Pascal!

Reviewed by Jorbin.
Merges [58068] to the 6.5 branch.

Props salcode, aslamdoctor, jorbin, swissspidy.
Fixes #60862.

Note: See TracTickets for help on using tickets.