Reconsider _doing_it_wrong warning when loading translations early

[TimothyBlynJacobs]

In #44937 we started emitting a _doing_it_wrong notice when a plugin calls a translation function before after_setup_theme.

It is almost always incorrect to load those translations early.

When called before pluggable.php is loaded, we don't yet have access to the current user, so the translations will never be loaded in the current user's domain in WP-Admin.

When called on plugins_loaded, the user will be forced to load early, which can lead to filters being called at unexpected times which can potentially lead to breakage.

The addition of the _doing_it_wrong warning has undoubtedly revealed some plugins doing things that are obviously wrong. The ​make post explains some of these.

However, it effectively prevents plugins from making a "best effort" chance at emitting a translated error message if they do need to kill execution early. For instance, in the context of a Security plugin, or when critical errors are encountered, and execution cannot continue.

From what I can tell, plugins are now forced to effectively emit messages in English to prevent a warning from being issued.

In Core, we have wp_load_translations_early to be able to make this best effort to communicate in a language the site owner can understand. I think ideally, we'd explore something similar that plugin authors could poptentially use to indicate they are also loading translations "early".

In lieu of that, I think we need to revert this in 6.7.1 and try again in 6.8 with such a mechanism in place.

I've tentatively milestoned this for 6.7.1 for visibility and discussion.

[domainsupport]

We would be in favour of rolling this back.

On some sites (that we monitor closely) our log files are filling up so much that we cannot easily see any genuine errors!

There is a fix out in the wild to use ...

<?php
add_filter( 'doing_it_wrong_trigger_error', '__return_false' );

... but this has no effect on some plugins, and we have had to modify line 1346 of /wp-includes/l10n.php ...

<?php
        if ( true === false && ! doing_action( 'after_setup_theme' ) && ! did_action( 'after_setup_theme' ) ) {

... in order to prevent this occurring!

Oliver

[swissspidy]

@TimothyBlynJacobs Do you have any actual widespread examples of this hypothetical use case? And why does temporarily hooking into existing filters to disable the warning or delaying until init not work there?

[TimothyBlynJacobs]

It isn't a hypothetical use case, but one I'm running into with one of the ​plugins I maintain. The fix that I've made, yet to release, is to just output those messages in English.

How widespread it is beyond me, I don't know. While it is certainly less prevalent than plugins unconditionally translating strings, it's certainly not 0. And I don't think a _doing_it_wrong notice should have false positives for valid use cases. General advice should be communicated in developer docs.

From what I can tell, that'd have to be done at the _doing_it_wrong level. doing_it_wrong_run always triggers which is a bit unfortunate as well.

If the consensus is that the noise is worth the gain, then this can of course be closed. But I wanted to get at least get a conversation going in one place.

[kowsar89]

It is pretty widespread. I've gathered the following list after a quick search:

1. Astra: ​https://wordpress.org/support/topic/function-_load_textdomain_just_in_time-was-called-incorrectly-22/
2. Yoast: ​https://github.com/Yoast/wordpress-seo/issues/21818
3. WPCode: ​https://wordpress.org/support/topic/function-_load_textdomain_just_in_time-was-called-incorrectly-12/
4. WP Rocket: ​https://github.com/wp-media/wp-rocket/issues/7102
5. WooCommerce: ​https://github.com/woocommerce/woocommerce/issues/52646

[benniledl]

Is the following behavior related to this ticket?

#61518: "Switch locale to admin locale when sending admin notifications"
Implemented:
When an email is sent to an admin, WP checks if there’s a user associated with the admin’s email address. If such a user exists, the email uses the locale set for that user.

On one of my sites, I noticed today. The site’s default language is ES, but the admin associated with the email address has their locale set to EN. Despite this, the automated plugin update notification email was sent in ES, not in the admin's locale (EN).

I have not created an ticket for this in trac yet because I thought maybe it is related to the timing of translations loading just as the issue in this ticket.

[kowsar89]

@domainsupport I'm using this workaround for now: ​https://gist.github.com/kowsar89/ed30f2b7abc5d4784ba4b05503c70fe0

[domainsupport]

Replying to kowsar89:

@domainsupport I'm using this workaround for now: ​https://gist.github.com/kowsar89/ed30f2b7abc5d4784ba4b05503c70fe0

Thanks for that. Although that's a better solution than the one I'd seen which instead turns all "doing it wrong" errors off, this still doesn't work in some instances. I'm assuming perhaps because add_action() is loaded on / after plugins_loaded and perhaps somehow translations are sometimes attempted to be loaded even earlier than that so it's still to late to catch the notification? That's just a hunch.

[swissspidy]

@TimothyBlynJacobs Do you have an issue on that plugin with a code reference so I can take a closer look? It‘s not clear to me why deferring until init or temporary disabling the notice doesn‘t work for you.

@kowsar89 All these plugins can simply fix their code. Most of them already did, so just update.

@benniledl No that‘s a separate ticket and implementation, as per the dev-note. Please open a new ticket if this keeps happening.

[LeonidasMilossis]

Replying to kowsar89:

It is pretty widespread. I've gathered the following list after a quick search:

1. Astra: ​https://wordpress.org/support/topic/function-_load_textdomain_just_in_time-was-called-incorrectly-22/
2. Yoast: ​https://github.com/Yoast/wordpress-seo/issues/21818
3. WPCode: ​https://wordpress.org/support/topic/function-_load_textdomain_just_in_time-was-called-incorrectly-12/
4. WP Rocket: ​https://github.com/wp-media/wp-rocket/issues/7102
5. WooCommerce: ​https://github.com/woocommerce/woocommerce/issues/52646

For what it's worth, today's release of Yoast SEO 23.9 should have this resolved.

[TimothyBlynJacobs]

Do you have an issue on that plugin with a code reference so I can take a closer look? It‘s not clear to me why deferring until init

@swissspidy It's kind of entangled. But for some issues, we were able to do this, or re-register on init to provide translations at that time. But the plugin contains firewall functionality, where we intentionally die early to prevent other plugins from running as early as mu-plugin load.

or temporary disabling the notice doesn‘t work for you.

I wish there was a filter in _load_textdomain_just_in_time to control this. But yeah, if the consensus is to not adjust this, then I think we'd have to hook into doing_it_wrong_trigger_error.

[danielhuesken]

When you don't use the load_plugin_textdomain() in the plugin and use an translations function "early" it will autoload the translation from core and trigger the doing_it_wrong_trigger_error.

So maybe the core function should also not load translations too early.

[desrosj]

Unfortunately, we're out of time to consider this for 6.7.1.

Let's try to reach a consensus for 6.7.2.

[swissspidy]

@TimothyBlynJacobs Have you tried simply calling load_textdomain() for your domain if you need it early? Not at the computer right now, but that should load the translation without any warnings.

[EusebiuOprinoiu]

The recommendation from _doing_it_wrong suggests using init as the right hook for loading translations.
The warning should recommend the use of init with a low priority. (preferably 0 or a negative number)
If developers use init without specifying a priority (meaning priority 10), there could be issues.

For example, post types and taxonomies are usually registered on init. This requires language files to be loaded because labels are translatable.

Another option would be to recommend after_setup_theme, but this could also be problematic because the user is not yet authenticated. (and the Site Language option from the user's profile is ignored)

[swissspidy]

Of course one should load translations before using translatable strings, that has always been the case. The JIT logic makes things easier though.

[EusebiuOprinoiu]

Yes. The problem is that many developers will follow your suggestion blindly and use init as a drop-in replacement for plugins_loaded to eliminate the warnings generated by _doing_it_wrong.

Because init is usually the earliest action where translatable strings are needed, and because init is often used without an explicit priority, my suggestion is to change the warning message with a recommendation that will cause fewer issues if followed blindly.

Current message:
Translation loading for the theme/plugin domain was triggered too early. This is usually an indicator for some code in the plugin or theme running too early. Translations should be loaded at the init action or later.

Possible improved message:
Translation loading for the theme/plugin domain was triggered too early. This is usually an indicator for some code in the plugin or theme running too early. Translations should be loaded at the after_setup_theme action or later. In most cases, init with priority 0 can be used without issues.

[brianhogg]

One potential case for reverting is the scheduling of jobs via wp_schedule_event. This often happens before init via plugins_loaded or even within a register_activation_hook. The scheduling causes the cron_schedules filter to fire, and those schedules have the display value which is usually translatable (ie. __( 'Every %s minutes', 'textdomain').

I'm not seeing a good workaround for this other than making the schedule display string english-only, or else moving the scheduling of the event to something after init and needing to do a wp_next_scheduled check or transient check every page load (instead of once via register_activation_hook).

[swissspidy]

Interesting scenario with the cron schedule. Is there a particular plugin where you have seen this? Or code to reproduce at least?

Note: Updating the ticket description to make it more clear that there are multiple possible outcomes of this ticket. Reverting ist just one of them, and not the best one (people would still write bad code)

[brianhogg]

The Events Calendar Pro causes this error with the majority of plugins that have a schedule.

Something like this should cause the warning since tecs_register_cron_schedule is run before init yet oddly I'm not seeing the translation warning on my local:

<?php
function tecs_schedule_event() {
    var_dump( 'scheduling...' );
    wp_schedule_event( time(), 'every_40mins', 'tecs_scheduled_event' );
}
add_action( 'plugins_loaded', 'tecs_schedule_event' );

function tecs_init() {
    var_dump( 'tecs_init' );
}
add_action( 'init', 'tecs_init' );

function tecs_register_cron_schedule( $schedules ) {
    var_dump( 'registering schedule...' );
    $schedules['every_40mins'] = [
        'interval' => 2400,
        'display' => __( 'Every 40 minutes', 'the-events-calendar-shortcode' ),
    ];

    return $schedules;
}
add_action( 'cron_schedules', 'tecs_register_cron_schedule' );

[swissspidy]

One issue there is that you're calling wp_schedule_event() (which calls wp_get_schedules() on plugins_loaded, so you are triggering a translation call on plugins_loaded. If you do it on init instead, I don't see any issue, be it when loading normally or when actually executing wp-cron.

[apermo]

In case this will be reverted, I suggest to keep the code, and just have a condition around it. So that production environments would not see this, but at least development would get to see this notice. So that plugin and theme developers can fix their code, before it is added again.

In our case, we have a sentry logging all notices etc on production, we have implemented a workaround, to suppress the notice, but there will be other companies which are spamming their error logs, or worse the frontend with this.

I think reverting is a valid option, but assuming it will come back, developers should be made aware of the change, at least on development

[brianhogg]

One issue there is that you're calling wp_schedule_event() (which calls wp_get_schedules() on plugins_loaded, so you are triggering a translation call on plugins_loaded. If you do it on init instead, I don't see any issue, be it when loading normally or when actually executing wp-cron.

Understood - it was just an example of what a lot of plugins are doing, including scheduling a task via register_activation_hook so they don't need to do a wp_next_scheduled check every request. That's no longer possible with a translatable display value.

If any plugin calls wp_schedule_event before init, it should cause all plugins that are registering a custom schedule to generate the warning.

[swissspidy]

In case this will be reverted, I suggest to keep the code, and just have a condition around it. So that production environments would not see this

That's what WP_DEBUG, WP_DEBUG_LOG, and WP_DEBUG_DISPLAY are for.

[mgparisi]

This is awful, half of my websites are affected, and I want to register a complaint. WordPress is acting like an application in a death spiral, and this is not acceptable.

I suggest adding this code to core:
php

function notify_wordpress_doing_it_wrong() {
    _doing_it_wrong(
        __FUNCTION__,
        'WordPress.org and WordPress.com, it seems like you might be "doing_it_wrong". Please review your operations and make improvements where necessary. Thanks!',
        '6.7.1'
    );
}
add_action( 'admin_notices', 'notify_wordpress_doing_it_wrong' );

Joke aside, I agree with @swissspidy this is what these variables are for. Except that for some reason this update changes default settings so that WP_DEBUG_DISPLAY now needs to be explicitly set. Which is where the complaints are coming from. New notice display defaults should not be changed in an application of this age. And we don't want it to default this way. Turning True on WP_Debug, should not default WP_Display_Notices to true. These notices are happening before the headers are sent, making it impossible for non-technical people to login to update the plugins. The solution "add_filter( 'doing_it_wrong_trigger_error', 'return_false' );" is not adequately stated in many of the documentation, as the documentation doesn't tell us where to put this.

Here is another one of these bug reports, but they are everywhere.

Spectra: ​https://wordpress.org/support/topic/update-to-wp6-7-_load_textdomain_just_in_time-was-called-incorrectly/

In addition, from my experience this also crashes wp-login.php, so even using that link won't get you logged in as that will cause the headers to fail. The only way a user can thus fix this is to modify code, or to disable all plugins, and that is not a good solution for the average business owner who has no idea how to code. Note: wp-login.php has always avoided errors, even when the entire site is down wp-login.php should work. This is a departure from that. Which is again why these variables should default to display = false

If this has the potential to also affect Cron, then that is also equally concerning. This was not well thought out solution. The problem with displaying messages in cron or in the REST API/Admin is that no one will see it, and many people won't notice these services are failing on their website for some time. Which is another reason displaying errors should default to false, even if wp_debug is true.

WordPress seems to be failing, and I'm not sure why. You are continuously costing your users money and time, and bringing down some of their most valuable resources. With me, I turned on "Auto Updates" so that I could save my customers money of manually updating the website. But if this is the norm, then I got to turn these off and start charging for monthly updates again. I'd prefer not to charge my customers more because WordPress is _doing_it_wrong()

I hope filing this ticket doesn't get me banned. With all the things going on, I wouldn't be surprised to see you all start banning anyone who criticizes the code coming out. We are investors in your company, and we have invested a lot. We should have you as a partner, not an enemy. Next up, a checkbox on the login that promises we should never speak negatively about wordpress. "Yes WordPress.org, your code never broke my website or cost me a lot of money, I was totally at fault. Thank you WordPress for your wonderful, holy code. We shall pray to you tonight, and sacrifice our first child in your name."

[jorbin]

I think the idea of improving the messaging pot forward in comments 15/17 might be the best path forward since as of now all the examples seem to be places where in fact, the code is doing it wrong.

[brianhogg]

I think the idea of improving the messaging pot forward in comments 15/17 might be the best path forward since as of now all the examples seem to be places where in fact, the code is doing it wrong.

Scheduling tasks with register_activation_hook is both common and valid. Otherwise, we'd need to add an extra check on every page load to see if the task has been scheduled or keep the display text in English only. This previously worked fine, which raises the question of why we're now forcing translations on or after init.

The ticket description also gives the example of a security plugin that could output messages early, or in the case of a message related to a critical error, which are now both locked to English.

If we want to continue with this, could we at least make it a notice that doesn't break translations while we consider better solutions? Otherwise, I think we should revert this change, evaluate what the workarounds would be in these valid scenarios, and then introduce a revised patch.

[swissspidy]

I already mentioned above that the issue with that cron example was the plugins_loaded part, not the scheduling part. I also haven't seen any other reports about this causing an issue with register_activation_hook, so if you could share a code example for that, that would be helpful.

Also, possible workarounds for security plugins and other cases were already mentioned, and add_filter( 'doing_it_wrong_trigger_error', '__return_false' ); always works as a last resort.

And to clarify, this change doesn't "break translations". It's just a notice. Translations still continue to be loaded, even if you do it too early.

After 6.7 things seem to have calmed down quite a bit and most big plugins which were previously doing it wrong fixed their code for the better. At this point I don't see why this should be outright reverted.

Sure, we can fix messaging and improve edge cases, but so far nothing indicates a revert to me. Which is why I previously changed the ticket title already.

[brianhogg]

And to clarify, this change doesn't "break translations". It's just a notice. Translations still continue to be loaded, even if you do it too early.

This doesn't appear to be true, from numerous support tickets and fixes I've been making since WP 6.7. When the notice is shown, the translations for that text domain no longer load in future included files. This is even more problematic when other plugins try to use a translation string from another plugin's text domain causing the translations to fail.

As an example, you can see this by installing and activating LifterLMS 7.8.6 on a Spanish site, and the translations for the setup wizard do not load apart from the strings for the "steps" in the header which are added too early via the constructor of the setup wizard class. Strings located in the setup-intro.php file (the body of the wizard) included later on in the page load do not work. With the latest version 7.8.7 these early strings are ​delayed until `init` and the translations in the body of the wizard load once again.

If it was just a notice that'd be great, it's the breaking of translations with no previous _doing_it_wrong notices in prior versions of WP that's the issue. Could we look into making this a warning only as a first step?

[swissspidy]

Again, the fact that a warning is emitted does not cause this behavior. Code execution isn't stopped, see ​https://github.com/WordPress/wordpress-develop/blob/c697356c29effcfa8115d852bc89385f7112c202/src/wp-includes/l10n.php#L1369-L1380.

If anything, it would be a side effect of [59157], which is a related change that was made.

The problem you encountered in LifterLMS 7.8.6 is that you loaded translations early and then unloaded them again in llms_load_textdomain(), which was called between the setup wizard steps at the top and the text at the bottom.

Sure we can look into optimizing [59157] (perhaps best in a new ticket with a clean slate) to sort of restore previous behavior, but if a plugin is clearly doing it wrong, a warning is still warranted. And if a plugin is unloading its own translations, then core can't really do anything about it.

[brianhogg]

@swissspidy Thanks for the detail, much appreciated!

I believe plugins like Loco Translate operate in a similar way, and the unload was added to account for certain translation plugins that specify custom language file locations. I'll re-test if the unload is still necessary. Regardless, I believe we've fixed the majority if not all of the instances where translation strings are used before init in the plugins I manage.

Will share any other examples I see either here or on a separate ticket with reference to 59157.

[jorbin]

Sure, we can fix messaging and improve edge cases, but so far nothing indicates a revert to me.

Completely agree with this. If this is something that is going to be included in 6.7.2, I think we need to see a proposed text update sooner rather than later. Otherwise, I think this is something that should be pushed to a later release.