`WP_Customize_Manager::prepare_setting_validity_for_js()` assumes `$error_data` is array

[dlh]

Previously discussed in Slack: ​https://wordpress.slack.com/archives/core-customize/p1472276384000282

@westonruter proposed two patches in that discussion to address the issue, both of which work fine in my testing. I can't yet come up with an overwhelming reason to choose one approach over the other.

Still, the attached patch goes with the first option, which seems more faithful to the original $data added to the WP_Error. It also seems to avoid an inconsistency where WP_Error data that is an array is placed alongside from_server => true but non-array data isn't, although that behavior might be desirable in a scenario I missed.

[westonruter]

@dlh I've been thinking about this more, and I think perhaps the (my) initial decision to add from_sever to the data sent from the server was faulty to begin with. It really shouldn't be mutating the data that was added to the WP_Error. What if instead we just add the from_server flag when we process the validities on the client when they are sent from the server? We could introduce a new fromServer property on wp.customize.Notification to represent this concretely.

What do you think of 37890.2.diff​?

(This also fixes a bug I noticed with regard to the setting property being completely ignored when the setting notification is synced to the control notifications.)

[dlh]

@westonruter 37890.2.diff​ looks good to me. While I don't have a real-world case to hand that I could test against the change to from_server, I agree with you generally about not mutating the error data.

One other thing I noticed while testing the patch: $data is optional in WP_Error::add(), but, in practice, it seems like the Customizer sometimes requires it because it compares the data property in JS before determining whether the notification needsReplacement. Just adding a different $message to the WP_Error isn't enough to replace the notification if $code is unchanged and $data is a blank string each time (or the same of whatever value).

Again, not a huge issue, and it might be intentional -- just took me a minute to realize why my error message wasn't changing.

[westonruter]

@dlh Good question. I think the idea on not checking for a modified notification.message is that generally the message would just be a human-readable version of the notification.code. But since this was unexpected for you, it's probably not something that should in fact be assumed. I've updated the patch to also check for a difference in message: 37890.3.diff​. If that works for you, I'll commit.

[dlh]

37890.3.diff​ also looks good. Thanks @westonruter!

[westonruter]

In 38513:

Customize: Fix php warning due to WP_Customize_Manager::prepare_setting_validity_for_js() incorrectly assuming that WP_Error will only ever have arrays in its $error_data.

• Eliminates the server mutating the a WP_Error's $error_data to merge-in a $from_server flag (since it may not be an array to begin with). Instead it defers to the client to add a fromServer param on any Notification instances created from server-sent errors.
• Ensures that notifications will be re-rendered if a notification's message changes but the data and type remain the same.
• Adds explicit support for the Notification class to have a setting property, ensuring that the property is set whereas previously it was dropped.

Fixes #37890.
Props westonruter, dlh.