Prevent translations from being loaded too early #47113

swissspidy · 2024-05-02T12:31:20Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Since WordPress 4.6, WordPress has supported so-called just-in-time translation loading. Whenever it encounters a __() call, it tries to load the translations for that domain if it hasn't already. That means one theoretically doesn't need load_plugin_textdomain() anymore, though it still can be used.

Ideally, this should only happen after the init hook, as that's when the current user has been fully set up in WordPress. This is important as users can set their locale in the user profile, which is used in the admin. Triggering translation loading before that means they will be loaded in the wrong locale. Unfortunately, some plugins are _doing_it_wrong() and load translations right when the plugin is executed, or upon plugins_loaded.

In my attempt to identify such plugins, I noticed that WooCommerce was a particularly stubborn case. There is a lot of code triggering just-in-time translation loading too early. This is particularly suboptimal because WooCommerce itself calls load_plugin_textdomain() later on on init, causing yet another translation call, which is just unnecessary at this point and not good for performance.

This PR is an attempt at identifying and eliminating all cases in WooCommerce triggering just-in-time translation loading before init.

This way, if core starts adding warnings for such cases, there won't be any warnings anymore for WooCommerce.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Some examples:

Remove autoloader

Set site language to German (de_DE)
Set user language to French (fr_FR)
Remove vendor/autoload_packages.php file so the missing autoloader warning kicks in
Visit dashboard
Without this PR: See autoloader admin notice in fr_FR, but error log entry in de_DE
With this PR: the error log entry is in en_US, as I removed the translation call

Temporarily remove load_plugin_textdomain() call

Set site language to German (de_DE)
Set user language to French (fr_FR)
Temporarily do an early return in \WooCommerce::load_plugin_textdomain() so that load_plugin_textdomain() isn't called
Visit some WooCommerce admin screen
Without this PR: Everything is in en_US or de_DE
With this PR: All strings are in fr_FR

With core PR

Apply Add warnings if loading translations too early WordPress/wordpress-develop#6462
Activate WooCommerce
See _doing_it_wrong() warnings

Changelog entry

Automatically create a changelog entry from the details below.
Adjust code loading order to prevent just-in-time translation loading from happening too early.

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement - Improvement to existing functionality

Message

Comment

github-actions · 2024-05-02T12:31:56Z

Hi , @woocommerce/mothra

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

ralucaStan · 2024-05-21T15:00:21Z

@swissspidy thank you for taking the time to work on this PR. There are some conflicts on your PR as there were some changes on the Additional fields side. Would you mind fixing those and updating the branch?

swissspidy · 2024-05-21T15:45:09Z

Sure, addressed now! 👍

coreymckrill

@swissspidy Thanks for this PR! I'm excited that this could potentially be a performance improvement for WooCommerce, and also improve how we implement translations.

I'm going to take another pass at this to do some specific testing (and others may as well) but I wanted to get some initial thoughts out here.

Switching a bunch of things from one action hook (plugins_loaded) to a later hook (init) makes me nervous because there are so many other plugins that integrate with WooCommerce, we can't really anticipate what assumptions they might be making about when things will be available, or what will break if we make a change like this. That said, it looks like several of these changes you make are in our Internal namespace, and so are probably relatively safe. I did note a couple that seem unnecessary because they don't look like they are actually triggering any gettext calls, but I could be wrong about that.

This is particularly suboptimal because WooCommerce itself calls load_plugin_textdomain() later on on init, causing yet another translation call, which is just unnecessary at this point and not good for performance.

This is a good call out, and it's something I had actually noticed just recently, and was wondering if we still needed it. I notice that you didn't remove it as part of this PR though. Do you think that would be worth doing here?

coreymckrill · 2024-05-21T21:52:53Z

plugins/woocommerce/includes/admin/helper/class-wc-helper-updater.php

@@ -26,7 +26,7 @@ public static function load() {
 		add_action( 'pre_set_site_transient_update_themes', array( __CLASS__, 'transient_update_themes' ), 21, 1 );
 		add_action( 'upgrader_process_complete', array( __CLASS__, 'upgrader_process_complete' ) );
 		add_action( 'upgrader_pre_download', array( __CLASS__, 'block_expired_updates' ), 10, 2 );
-		add_action( 'plugins_loaded', array( __CLASS__, 'add_hook_for_modifying_update_notices' ) );
+		add_action( 'init', array( __CLASS__, 'add_hook_for_modifying_update_notices' ) );


🤔 Does this one really need to change? Looks like it doesn't actually use any gettext functions in the callback, just queues up a callback on another action hook that's after init...

You're right, it's not. I was getting fatal errors from WC_Woo_Update_Manager_Plugin::is_plugin_active() though because is_plugin_active_for_network wasn't yet available. That can be addressed better.

I changed this in 2fad07c, but not happy with it.

Actually, it would be even better to change this to admin_init, because the hooks there are only relevant for the admin, as they add admin notices.

Changed in c5c3e26

plugins/woocommerce/includes/class-woocommerce.php

coreymckrill · 2024-05-21T22:22:11Z

plugins/woocommerce/src/Blocks/Domain/Services/CheckoutFields.php

+	/**
+	 * Returns the keys of all core fields.
+	 *
+	 * @return array An array of field keys.
+	 */
+	public function get_core_fields_keys() {
+		return [
+			'email',
+			'first_name',
+			'last_name',
+			'company',
+			'address_1',
+			'address_2',
+			'country',
+			'city',
+			'state',
+			'postcode',
+			'phone',
+		];
+	}
+


I see why this is added, so that fields_locations can be populated in the constructor, but it's obviously not ideal since it means the keys have to be updated in two places if they change. But, since both the core_fields and the fields_locations properties in this class are set to private, we could do the same treatment for fields_locations, i.e. create a new private method called get_fields_locations and use that instead of referencing the class property. Then you could remove the property assignment from the class constructor.

Agreed, the duplication is not ideal. A slight difference here is that fields_locations can be mutated via register_checkout_field()/deregister_checkout_field. Plus, it's accessed in quite a few places throughout the class. Any suggestions on how best to handle that?

plugins/woocommerce/src/Internal/Integrations/WPConsentAPI.php

swissspidy · 2024-05-28T15:42:00Z

I think I resolved the Packages issue without re-introducing the translation issue.

The stack trace is basically this:

Packages -> \Automattic\WooCommerce\Blocks\Package -> \Automattic\WooCommerce\Blocks\Domain\Bootstrap -> ShippingController -> LocalPickupUtils::is_local_pickup_enabled() -> __()

The current workaround works, but makes it easy to call too early translation calls elsehwere, because everything in Packages runs so early.

The init_jetpack_connection_config case I "fixed" by not calling a translation function. Not great, but a better fix requires a change in that package.

swissspidy · 2024-06-03T14:07:09Z

@coreymckrill @senadir can someone help approve the workflow runs here?

senadir · 2024-06-03T14:09:47Z

Closing and reopening to force them to start.

senadir · 2024-06-03T14:20:07Z

I did approve them before closing and opening, just did that again.

senadir

I reviewed this yesterday and everything seems to be working fine in Checkout flows, what's left is more testing with some plugins and figuring out why tests aren't passing.

swissspidy · 2024-06-09T12:20:51Z

Tests should be passing now, but they need to be approved again.

senadir · 2024-06-09T18:14:23Z

FWIW Build Live Branch / build (pull_request) will continue to fail for now until we fix it in trunk (it has an issue with external authors).

senadir · 2024-06-09T18:30:22Z

I took anther look at the PR code and it's honestly not touching any critical stuff, the main stuff I can think about are the blocks related code (which I tested) + order attribution (with @layoutd tested).

I think this is good to go but I'd love for us to avoid reintroducing this in the future. @swissspidy is it possible for us to add a unit test that would check if translation is loaded too early and will fail if so? If you're ensure how to write the test, just an example of how that hook would look like is enough for me.

swissspidy · 2024-06-17T12:57:37Z

Discussed this with @senadir in person at WCEU:

I am planning to add warnings for translations being loaded too early in WP 6.7, see WordPress/wordpress-develop#6462 and https://core.trac.wordpress.org/ticket/44937. This will make it easy to detect regressions in the future. Until then there is not a very good way to test this.

So my suggestion would be to merge this for now, accepting the small risk of a regression between now and WP 6.7.

senadir · 2024-06-19T10:17:41Z

Hey @swissspidy I discussed with internally with the release team and while I feel confident we can merge this, merging this today means it will land on WooCommerce 9.1 and won't get a lot of testing time, so I'd wait and merge it tomorrow so it lands on 9.2 and gives it even more time in local developer devs and testing time.

* Prevent translations from being loaded too early * Undo accidental changes to `CheckoutFields` * Keep plugins_loaded, but include required file * Update order in the meantime * Change to admin_init * Add changelog file * Undo `init_jetpack_connection_config` change * Undo change to Packages class * Fix LocalPickupUtils instead * PHPCS fixes * Update method name in test

Prevent translations from being loaded too early

a36d696

github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution labels May 2, 2024

woocommercebot requested review from a team, jorgeatorres, alexflorisca, gigitux, chihsuan and moon0326 and removed request for a team May 2, 2024 12:31

swissspidy mentioned this pull request May 2, 2024

Add warnings if loading translations too early WordPress/wordpress-develop#6462

Closed

6 tasks

Undo accidental changes to CheckoutFields

b6cc42a

adrianduffell removed request for a team, chihsuan and moon0326 May 18, 2024 01:44

alexflorisca removed their request for review May 21, 2024 13:53

ralucaStan requested a review from senadir May 21, 2024 15:19

Merge branch 'trunk' into fix/l10n-too-early

7d83ad0

coreymckrill requested review from coreymckrill and removed request for jorgeatorres May 21, 2024 16:45

coreymckrill reviewed May 21, 2024

View reviewed changes

swissspidy added 3 commits May 28, 2024 17:15

Undo change to Packages class

6cb82ec

Fix LocalPickupUtils instead

48709aa

Merge branch 'trunk' into fix/l10n-too-early

2857bc2

Merge branch 'trunk' into fix/l10n-too-early

866a40d

swissspidy requested a review from senadir May 29, 2024 08:54

Merge branch 'trunk' into fix/l10n-too-early

7012d40

senadir closed this Jun 3, 2024

senadir reopened this Jun 3, 2024

This comment was marked as resolved.

Sign in to view

senadir approved these changes Jun 6, 2024

View reviewed changes

swissspidy added 3 commits June 9, 2024 14:13

Merge branch 'trunk' into fix/l10n-too-early

c028c72

PHPCS fixes

0360f43

Update method name in test

6c3fa71

juliaamosova added the impact: high To be added by the PR author/reviewer if the PR could very likely cause SEV-1 or SEV-2 issues label Jun 18, 2024

senadir merged commit e5b7f84 into woocommerce:trunk Jun 25, 2024
38 of 40 checks passed

github-actions bot added this to the 9.2.0 milestone Jun 25, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Jun 25, 2024

rodelgc added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent translations from being loaded too early #47113

Prevent translations from being loaded too early #47113

swissspidy commented May 2, 2024

github-actions bot commented May 2, 2024 •

edited

Loading

ralucaStan commented May 21, 2024

swissspidy commented May 21, 2024

coreymckrill left a comment

coreymckrill May 21, 2024

swissspidy May 22, 2024

swissspidy May 22, 2024

swissspidy May 22, 2024

coreymckrill May 21, 2024

swissspidy May 22, 2024

swissspidy commented May 28, 2024

swissspidy commented Jun 3, 2024

senadir commented Jun 3, 2024

This comment was marked as resolved.

senadir commented Jun 3, 2024

senadir left a comment

swissspidy commented Jun 9, 2024

senadir commented Jun 9, 2024

senadir commented Jun 9, 2024 •

edited

Loading

swissspidy commented Jun 17, 2024

senadir commented Jun 19, 2024

Prevent translations from being loaded too early #47113

Prevent translations from being loaded too early #47113

Conversation

swissspidy commented May 2, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented May 2, 2024 • edited Loading

ralucaStan commented May 21, 2024

swissspidy commented May 21, 2024

coreymckrill left a comment

Choose a reason for hiding this comment

coreymckrill May 21, 2024

Choose a reason for hiding this comment

swissspidy May 22, 2024

Choose a reason for hiding this comment

swissspidy May 22, 2024

Choose a reason for hiding this comment

swissspidy May 22, 2024

Choose a reason for hiding this comment

coreymckrill May 21, 2024

Choose a reason for hiding this comment

swissspidy May 22, 2024

Choose a reason for hiding this comment

swissspidy commented May 28, 2024

swissspidy commented Jun 3, 2024

senadir commented Jun 3, 2024

This comment was marked as resolved.

senadir commented Jun 3, 2024

senadir left a comment

Choose a reason for hiding this comment

swissspidy commented Jun 9, 2024

senadir commented Jun 9, 2024

senadir commented Jun 9, 2024 • edited Loading

swissspidy commented Jun 17, 2024

senadir commented Jun 19, 2024

github-actions bot commented May 2, 2024 •

edited

Loading

senadir commented Jun 9, 2024 •

edited

Loading