Skip to content

Conversation

@cadic
Copy link

@cadic cadic commented Nov 10, 2022

In [53874] the $locale parameter was added to load_textdomain() so it can be used to properly fill WP_Textdomain_Registry.

Since the $locale may not be the same value as determine_locale() returns (e.g. when filtered by plugin_locale in load_plugin_textdomain()) we should also pass the new parameter to the filter so custom file loading implementations are using the same locale as load_textdomain() got.

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@ocean90 ocean90 requested a review from swissspidy November 10, 2022 13:03
@ocean90 ocean90 changed the title Pass to override_load_textdomain filterUpdate docs Pass $locale to override_load_textdomain filter Nov 10, 2022
Co-authored-by: Dominik Schilling <[email protected]>
@cadic cadic requested review from ocean90 and removed request for swissspidy November 11, 2022 14:34
@cadic cadic requested review from costdev and removed request for ocean90 January 22, 2023 15:22
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks for the updates @cadic!

@SergeyBiryukov
Copy link
Member

Would it make sense to move the locale detection earlier in the function, so that it's always passed as a string to the filter, instead of string|null?

if ( ! $locale ) {
	$locale = determine_locale();
}

@swissspidy
Copy link
Member

swissspidy commented Jan 26, 2023

IIRC that get_locale() call is deliberately as late as possible to avoid too many calls for performance reasons.

@audrasjb
Copy link
Contributor

audrasjb commented Feb 2, 2023

@audrasjb audrasjb closed this Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants