Make WordPress Core

Opened 10 years ago

Last modified 2 years ago

#39295 new enhancement

Prevent infinite loop when calling get_user_locale() in a 'locale' filter

Reported by: yoavf Owned by:
Priority: normal Milestone: Awaiting Review
Component: I18N Version: 4.7
Severity: major Keywords: has-patch 2nd-opinion
Cc: Focuses:

Description

Using get_user_locale() in a locale filter (for get_locale()) might sound like a good idea, for example to change the site locale depending on our user.

<?php
        if ( $condition) {
                add_filter( 'locale', 'get_user_locale' );
        }

However, if one does that and a non-logged in user (or a user without a locale value) visits the site, a infinite loop will be triggered, since get_user_locale() will itself call get_locale() when no user->locale value exists.

Since this only affects logged out users, it feels like an easy trap to miss, and we should prevent that.

Attachments (3)

39295.diff​ (1.4 KB ) - added by yoavf 10 years ago.
39295.2.diff​ (1.7 KB ) - added by SergeyBiryukov 6 years ago.
39295.2.alt.diff​ (1.8 KB ) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (7)

@yoavf
10 years ago

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


6 years ago

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


6 years ago

#3 @SergeyBiryukov
6 years ago

39295.2.diff​ is a refresh that follows the approach of 39295.diff​ with some improvements:

  • Simplified logic for consistency with get_locale().
  • Moved the filter name out of the translatable string.
  • The test performs the assertions after restoring the current locale, to avoid affecting other tests in case of failure.

39295.2.alt.diff​ is an alternative patch that just makes add_filter( 'locale', 'get_user_locale' ) work as expected, without the _doing_it_wrong() message, by using the existing $locale global as a fallback to avoid the infinite loop. Perhaps that could be the preferred fix here?

Version 1, edited 6 years ago by SergeyBiryukov (previous) (next) (diff)

#4 @swissspidy
2 years ago

  • Keywords 2nd-opinion added

Is this still relevant? IMHO it should be up to the caller to unhook their callback in such a case.

Note: See TracTickets for help on using tickets.