Make WordPress Core

Opened 5 months ago

Closed 2 months ago

Last modified 12 hours ago

#62244 closed enhancement (fixed)

Just-in-time translation loading for plugins/themes not in the directory

Reported by: swissspidy's profile swissspidy Owned by: swissspidy's profile swissspidy
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch has-dev-note
Focuses: Cc:

Description

This is a follow-up to the just-in-time translation loading we added in WordPress 4.6, see #34114.

We could theoretically expand this to custom plugins as well.

If no translation files are found for a given domain, we could use get_plugins() and check if there's a plugin with a matching slug, get its Domain Path value (like _get_plugin_data_markup_translate does) and then store that location in WP_Textdomain_Registry.

This way, custom plugins won't need to call load_plugin_textdomain() anymore as long as they define Domain Path in the plugin headers.

Question is whether we can do this in a performant way, minimizing file reads, adding caching, etc.

Change History (37)

This ticket was mentioned in PR #7592 on WordPress/wordpress-develop by @swissspidy.


5 months ago
#1

  • Keywords has-patch added

Now works for custom themes and plugins with a Domain Path

This way, themes and plugins will not have to use load_theme_textdomain() or load_plugin_textdomain() anymore, as WordPress knows where to look for the files and will load the translations automatically the first time __() is encountered.

There's a minor hit for the file read caused by get_plugin_data() but that should be minor.

Downside: loads get_plugin_data() / wp-admin/includes/plugin.php in wp-settings.php, making a lot of functions available earlier than usual.

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

#2 @swissspidy
5 months ago

  • Milestone changed from Awaiting Review to 6.8

#3 @swissspidy
4 months ago

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

In 59461:

I18N: Load translations just-in-time for custom themes and plugins.

In #34114, just-in-time (JIT) translation loading was implemented for projects hosted on WordPress.org. This is now expanded to all other plugins/themes.

Projects with a custom Text Domain and Domain Path header no longer need to call load_plugin_textdomain() or load_theme_textdomain().

This reduces the risk of calling them too late, after some translation calls already happened, and generally makes it easier to properly internationalize a plugin or theme.

This moves the get_plugin_data() from wp-admin/includes/plugin.php to wp-includes/functions.php so it's available during the plugin loading process.

Props swissspidy.
Fixes #62244.

#5 @swissspidy
4 months ago

  • Keywords needs-dev-note added

#6 @johnbillion
4 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Changeset [59461] is causing the Airplane Mode plugin on my local development site to trigger a fatal error. It might be an indication that the plugin is doing it wrong, or it might be a bigger problem:

PHP Fatal error:  Uncaught Error: Call to undefined function is_plugin_active() in wp-content/plugins/airplane-mode/airplane-mode.php:162
Stack trace:
#0 wp-content/plugins/airplane-mode/airplane-mode.php(250): Airplane_Mode_Core->__construct()
#1 wp-content/plugins/airplane-mode/airplane-mode.php(1317): Airplane_Mode_Core::getInstance()
#2 wp-settings.php(526): include_once('wp...')
#3 /var/www/wp-config.php(108): require_once('wp...')
#4 wp-load.php(55): require_once('/var/www/wp-con...')
#5 wp-blog-header.php(13): require_once('wp...')
#6 _index.php(17): require('wp...')
#7 index.php(19): require_once('_i...')
#8 {main}
  thrown in wp-content/plugins/airplane-mode/airplane-mode.php on line 162

Reopening to investigate.

Last edited 4 months ago by johnbillion (previous) (diff)

#7 @swissspidy
4 months ago

I couldn't reproduce this at first, but then I realized you are using the current version from GitHub (0.2.7), which is newer than the one from the plugin directory (0.2.5).

The problem in that plugin is that it does the following:

if ( ! function_exists( 'get_plugin_data' ) ) {
	require_once ABSPATH . 'wp-admin/includes/plugin.php';
}

if ( is_plugin_active( 'jetpack/jetpack.php' ) ) {
	$jetpack_plugin = get_plugin_data( trailingslashit( WP_PLUGIN_DIR ) . 'jetpack/jetpack.php' );
}

The problem is that get_plugin_data() is now already defined, so the file isn't included. However, get_plugin_data is only defined in that file, hence the fatal error about the function not existing.

The proper way would be:

if ( ! function_exists( 'get_plugin_data' ) || ! function_exists( 'is_plugin_active' ) ) {
	require_once ABSPATH . 'wp-admin/includes/plugin.php';
}

I'll try find out if more plugins do it wrong like this (e.g. with GitHub Search), but that's gonna be difficult I think.

Patching Airplane Mode with the above change makes sense either way.

#8 @dd32
4 months ago

This has also caused some fatals on wp-themes.com, although I was struggling to understand how, it's the same type of code as in comment 7, get_plugin_data() is checked for and if not present plugin.php is loaded, and elsewhere in the theme is_plugin_active() is called.

Example: https://themes.trac.wordpress.org/ticket/182411#comment:2

Here's some affected themes:

Sidenote: Making is_plugin_active() always available would be a benefit to a bunch of various plugins/themes.

#9 @swissspidy
4 months ago

Thanks!

Seems like it‘s common to use both functions together or only check for one function while needing both. Relocating is_plugin_active() would help—unless there are more functions people use without checking.

This ticket was mentioned in PR #7900 on WordPress/wordpress-develop by @swissspidy.


4 months ago
#11

Plugins: Make more plugin-related functions available early on.

This is a follow-up to [59461], which moved `get_plugin_data()` from `wp-admin/includes/plugin.php` to `wp-includes/functions.php` so it's available during the plugin loading process.

Related functions like `is_plugin_active()` are often used together and should therefore be moved as well, to improve backward compatibility for plugins which load `wp-admin/includes/plugin.php` only conditionally.

Props johnbillion, dd32, swissspidy
Fixes #62244.

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

#12 @swissspidy
4 months ago

#62581 was marked as a duplicate.

#13 @swissspidy
4 months ago

Another error reported in #62581 by @neo2k23:

Fatal error: Uncaught TypeError: basename(): Argument #1 ($path) must be of type string, Theme given in /home/public_html/wp-settings.php:686 Stack trace: #0 /home/public_html/wp-settings.php(686): basename(Object(Theme)) #1 /home/wp-config.php(96): require_once('/home/...') #2 /home/public_html/wp-load.php(55): require_once('/home/..') #3 /home/public_html/wp-admin/admin.php(34): require_once('/home/...') #4 /home/public_html/wp-admin/about.php(10): require_once('/home/...') #5 {main} thrown in /home/public_html/wp-settings.php on line 686

wp_get_active_and_valid_themes() is supposed to return an array of strings. Theme is not a class that exists in WordPress.

@neo2k23 are you using some special plugin/theme/drop-in that modifies the $wp_stylesheet_path or $wp_template_path globals or so? Where is the Theme class coming from in your project?

#14 @neo2k23
4 months ago

I am using a theme from themeforest. wp_stylesheet_path and wp_template_path are not modified by the theme or even called. No code to be found in the theme code.

Turned off all plugins. Same result.

Last edited 4 months ago by neo2k23 (previous) (diff)

#15 @neo2k23
4 months ago

The theme has this code in the functions.php

<?php
if(!class_exists('Theme')){
        /* Load the Theme class. */
        require_once (TEMPLATEPATH . '/framework/theme.php');

        $theme = new Theme();
        $options = include(TEMPLATEPATH . '/framework/info.php');

        $theme->init($options);
}

#16 @neo2k23
4 months ago

Here is the complete code of the functions.php

<?php
<?php 
/**
 * Theme framework initialization
 *
 * Sets up the theme and provides some helper functions.
 *
 * This file will not be overridden by theme updates. So you can add your custom functions down below where indicated and they are safe. 
 *
 * When using a child theme (see http://codex.wordpress.org/Theme_Development and
 * http://codex.wordpress.org/Child_Themes), you can override certain functions
 * (those wrapped in a function_exists() call) by defining them first in your child theme's
 * functions.php file. The child theme's functions.php file is included before the parent
 * theme's file, so the child theme functions would be used.
 * This file is like a child functions file, and serves the same purpose, without having to go to the 
 * trouble of implementing a child theme, if only having one or two simple custom functions.
 *
 * NOTE: IF UPDATING THE THEME BY FTP, RENAME THIS FILE SO THAT IT IS NOT OVERRIDDEN OR DO NOT ALLOW
 * IT TO BE REPLACED IN THE FTP UPDATE.  IF SAVED BY A RENAME, OF COURSE REMEMBER TO COME BACK AND 
 * COPY ITS CUSTOM FUNCTIONS TO YOUR NEW FUNCTIONS.PHP FILE.
 */

if(!class_exists('Theme')){
        /* Load the Theme class. */
        require_once (TEMPLATEPATH . '/framework/theme.php');

        $theme = new Theme();
        $options = include(TEMPLATEPATH . '/framework/info.php');

        $theme->init($options);
}

$theme is not called global but if i checked it before the class exists it already contains the path to the theme.

I can change it to any other name and that fixes the issue. I guess more custom themes could run into this.

Last edited 4 months ago by neo2k23 (previous) (diff)

This ticket was mentioned in PR #7901 on WordPress/wordpress-develop by @swissspidy.


4 months ago
#17

`
I18N: Do not reuse $theme variable name after loading a theme's functions.php file.

The file could declare its own $theme variable, which would override the one used in the foreach loop.

To prevent this, call wp_get_theme() before loading the file and store the instance in a different variable.

Props neo2k23, swissspidy.
See #62244.

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

#18 follow-up: @swissspidy
4 months ago

Ah! That explains it! Thanks for sharing.

The theme defines $theme in its functions.php file, which overrides the $theme variable from the foreach loop.

Here's a PR that fixes this: https://github.com/WordPress/wordpress-develop/pull/7901

BackuPs commented on PR #7901:


4 months ago
#19

@swissspidy

Thank you. That fixes my issue.

#20 in reply to: ↑ 18 @neo2k23
4 months ago

Replying to swissspidy:

Ah! That explains it! Thanks for sharing.

The theme defines $theme in its functions.php file, which overrides the $theme variable from the foreach loop.

Here's a PR that fixes this: https://github.com/WordPress/wordpress-develop/pull/7901

Thank you for the fix. Myy issue is resolved. Although i had already altered $theme to $theme_init. :-)

BackuPs commented on PR #7901:


4 months ago
#21

@swissspidy

Thank you. That fixes my issue.

#22 @swissspidy
4 months ago

In 59466:

I18N: Do not reuse $theme variable name after loading a theme's functions.php file.

The file could declare its own $theme variable, which would override the one used in the foreach loop.

To prevent this, call wp_get_theme() before loading the file and store the instance in a different variable.

Props neo2k23, swissspidy.
See #62244.

#24 @swissspidy
4 months ago

With that settled, I think we also need to add this if using a child theme:

if ( $wp_theme->parent() ) {
        $wp_theme->parent()->load_textdomain();
}

Edit: actually, looks like that's done automatically.

Last edited 4 months ago by swissspidy (previous) (diff)

#25 @swissspidy
4 months ago

In 59479:

Plugins: Make more plugin-related functions available early on.

This is a follow-up to [59461], which moved get_plugin_data() from wp-admin/includes/plugin.php to wp-includes/functions.php so it's available during the plugin loading process.

Related functions like is_plugin_active() are often used together and should therefore be moved as well, to improve backward compatibility for plugins which load wp-admin/includes/plugin.php only conditionally.

Props johnbillion, dd32, swissspidy.
See #62244.

#27 follow-up: @peterwilsoncc
4 months ago

@swissspidy [59479] is causing fatal errors for some plugins, I am seeing it with WooCommerce subscriptions. On the WordPress/WordPress repo, 8f08a8e2e1 (the prior commit) is running fine but once I checkout 789d1d9c2e9 (the commit above), I am getting fatal errors

Stack trace is as follows:

[04-Dec-2024 01:10:47 UTC] PHP Fatal error:  Uncaught Error: Call to undefined function get_plugins() in /vagrant/content/plugins/woocommerce-subscriptions/includes/class-wc-subscriptions-dependency-manager.php:109
Stack trace:
#0 /vagrant/content/plugins/woocommerce-subscriptions/includes/class-wc-subscriptions-dependency-manager.php(73): WC_Subscriptions_Dependency_Manager->get_woocommerce_active_version()
#1 /vagrant/content/plugins/woocommerce-subscriptions/includes/class-wc-subscriptions-dependency-manager.php(45): WC_Subscriptions_Dependency_Manager->is_woocommerce_version_supported()
#2 /vagrant/content/plugins/woocommerce-subscriptions/woocommerce-subscriptions.php(39): WC_Subscriptions_Dependency_Manager->has_valid_dependencies()
#3 /vagrant/wp-build/wp-settings.php(526): include_once('/vagrant/conten...')
#4 phar:///usr/local/src/wp-cli/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1374): require('/vagrant/wp-bui...')
#5 phar:///usr/local/src/wp-cli/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1293): WP_CLI\Runner->load_wordpress()
#6 phar:///usr/local/src/wp-cli/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\Runner->start()
#7 phar:///usr/local/src/wp-cli/bin/wp/vendor/wp-cli/wp-cli/php/bootstrap.php(83): WP_CLI\Bootstrap\LaunchRunner->process(Object(WP_CLI\Bootstrap\BootstrapState))
#8 phar:///usr/local/src/wp-cli/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php(32): WP_CLI\bootstrap()
#9 phar:///usr/local/src/wp-cli/bin/wp/php/boot-phar.php(20): include('phar:///usr/loc...')
#10 /usr/local/src/wp-cli/bin/wp(4): include('phar:///usr/loc...')
#11 {main}
  thrown in /vagrant/content/plugins/woocommerce-subscriptions/includes/class-wc-subscriptions-dependency-manager.php on line 109

#28 in reply to: ↑ 27 @dd32
4 months ago

Replying to peterwilsoncc:

@swissspidy [59479] is causing fatal errors for some plugins

Without looking at the code, I'm guessing this is the exact same issue as above, but for a different function. Probably conditionally looking for is_plugin_active() and then assuming get_plugins() is loaded too.

It looks like that's indeed the case:

                // Load plugin.php if it's not already loaded.
                if ( ! function_exists( 'is_plugin_active' ) ) {
                        require_once ABSPATH . 'wp-admin/includes/plugin.php';
                }

                // Loop through all active plugins and check if WooCommerce is active.
                foreach ( get_plugins() as $plugin_slug => $plugin_data ) {

Here's a theme with the same problem:
https://themes.trac.wordpress.org/browser/gadgethub/1.0.6/includes/omnipress-theme-notice/class-theme-notice.php?marks=73,74,84#L65
(A whole bunch of similar themes too)

Last edited 4 months ago by dd32 (previous) (diff)

#29 follow-up: @swissspidy
4 months ago

Thanks!

While I don't like it, I feel like at this point it would be safer to just always include require_once ABSPATH . 'wp-admin/includes/plugin.php'; in wp-settings.php. Otherwise we play whac-a-mole trying to move functions every time we encounter a plugin at this. Loading the whole file (with a few unrelated functions) early is not great, but better than causing fatal errors.

This ticket was mentioned in PR #7945 on WordPress/wordpress-develop by @swissspidy.


4 months ago
#30

Plugins: Load `wp-admin/includes/plugin.php` earlier.

Partially reverts [59479] and [59461], which previously tried to move some functions from `wp-admin/includes/plugin.php` to `wp-includes/functions.php` so they are available early, so that `get_plugin_data()` can be used.

However, other functions from that file are often used by plugins without necessarily checking whether they are available, easily causing fatal errors. Requiring this file directly is a safer approach to avoid such errors.

Props peterwilsoncc, dd32, swissspidy.
See #62244.

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

#31 in reply to: ↑ 29 @dd32
4 months ago

Replying to swissspidy:

While I don't like it, I feel like at this point it would be safer to just always include require_once ABSPATH . 'wp-admin/includes/plugin.php'; in wp-settings.php.

Sadly, I do tend to agree.

#32 @swissspidy
4 months ago

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

In 59488:

Plugins: Load wp-admin/includes/plugin.php earlier.

Partially reverts [59479] and [59461], which previously tried to move some functions from wp-admin/includes/plugin.php to wp-includes/functions.php so they are available early, so that get_plugin_data() can be used.

However, other functions from that file are often used by plugins without necessarily checking whether they are available, easily causing fatal errors. Requiring this file directly is a safer approach to avoid such errors.

Props peterwilsoncc, dd32, swissspidy, johnbillion.
Fixes #62244.

#34 @swissspidy
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The registry call should happen before the plugin is loaded, so that the following case also triggers a warning:

<?php
/*
 * Plugin Name: Test
 * Domain Path: /languages
 * Text Domain: test
 */

__( 'Test 1', 'test' ); // This should trigger a warning.

This ticket was mentioned in PR #8153 on WordPress/wordpress-develop by @swissspidy.


2 months ago
#35

This is a follow-up to the original implementation.

I18N: Set textdomain registry information before loading plugins/theme.

This way, warnings for early translation calls can be emitted that aren't attached to any hook.

Follow-up to [59461].

Props swissspidy.
Fixes #62244. See #44937.

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

#36 @swissspidy
2 months ago

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

In 59670:

I18N: Set textdomain registry information before loading plugins/theme.

This way, warnings for early translation calls can be emitted that aren't attached to any hook.

Follow-up to [59461].

Props swissspidy.
Fixes #62244.See #44937.

#37 @JeffPaul
12 hours ago

  • Keywords has-dev-note added; needs-dev-note removed

Note that the dev note has been published as Internationalization improvements in 6.8.

Note: See TracTickets for help on using tickets.