[wp-trac] [WordPress Trac] #57427: WP_Locale doesn't initialize property arrays before using them

[WordPress Trac]

#57427: WP_Locale doesn't initialize property arrays before using them
--------------------------+-----------------------------
 Reporter:  tyxla         |       Owner:  hellofromTonya
     Type:  defect (bug)  |      Status:  accepted
 Priority:  normal        |   Milestone:  6.2
Component:  I18N          |     Version:  6.1
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:
--------------------------+-----------------------------

Comment (by hellofromTonya):

 Follow-up to my previous comment. I spent some time today re-reading the
 discussion and investigating the problem.

 === tl;dr
 * Initializing the properties at the definition point is a good design
 practice.

 * But something else is going on at the point where Gutenberg's code
 throws the `array_values() expects parameter 1 to be array, null given`
 notice|warning.

 * I was not able to reproduce the notice|warning.

 === More details

 ==== Thrown when attempting to use, not when setting.

 {{{
 Warning: array_values() expects parameter 1 to be array, null given
 }}}

 This notice|warning happens when ''using'' a non-array variable as an
 `array`. Notice, it says the property's data type was `null`, not `array`.

 It does ''not'' throw the notice|warning when setting. Why? PHP will type
 juggle from `null` to `array` before setting the element (as it does in
 `WP_Locale::init()`). See it in action here
 https://3v4l.org/m1QUYX#v7.4.33 and https://3v4l.org/PtmGf#v7.4.33.

 ==== Something else is happening

 When instantiating `WP_Locale`, `WP_Locale::init()` runs from the
 constructor. So invoking `new WP_Locale` automatically sets the properties
 to an `array` data type. For the notice|warning to happen:

 * the properties would have been ''reset'' to `null` somewhere in the call
 stack
 * Or a different object is in the global `$wp_locale`.

 For the latter, maybe the object inherits from `WP_Locale` and overloads
 its constructor or `init()`, thus ''not presetting'' the properties.

 If the `$wp_locale` global is an instance of `WP_Locale` and the
 properties were not reset, then the properties are `array` type and the
 warning|notice should not occur.

 So something else is going on.

 I'm curious of how to reproduce it for further investigation into the call
 stack.

 ==== Why is initializing the properties a good design practice?


 I agree with @azaozz and @tyxla about pre-defining a default state of the
 `WP_Locale` properties. Why?

 * The documentation and code are clearly expecting these properties to be
 an `array` data type.

 * Setting each to a default `array()` state further helps to clearly
 communicate the code design.

 * If another class inherits from `WP_Locale` but does not run `init()`
 from the constructor, then the properties would be `null` instead of
 `array`. Given that this is not a `final` class, it is possible that some
 real world sites and applications could be doing something custom. By
 initializing the properties as the patch does, any class extending from
 `WP_Locale` gains the benefits of the default value.

 === Hiding the problem

 >Would removing this warning make it harder to determine why the locale
 data is not loaded?

 [#comment:6 As @azaozz noted], it could obfuscate it. However, it is also
 possible that it resolves the issue ''if'' a custom implementation is
 being used OR the properties are being reset on purpose.

 === Next steps

 * IMO the patch (once a few tweaks are done) should be committed (as noted
 above).
 * A deeper investigative dive is needed into any impacted sites to better
 understand what's different on these sites.
    * Is the object in `GLOBALS['wp_locale']` an instance of `WP_Locale`?
    * If yes, then search for where the properties are being reset.
    * If no, look at the object's class code to determine how it presets
 the properties.
 * Step-by-step "how to reproduce instructions" are needed to help
 contributors investigate if this is a Core issue.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/57427#comment:9>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform