Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#37890 closed defect (bug) (fixed)

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

Reported by: dlh's profile dlh Owned by: westonruter's profile westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch
Focuses: Cc:


Previously discussed in Slack:

@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.

Attachments (3)

37890.patch (480 bytes) - added by dlh 8 years ago.
37890.2.diff (3.5 KB) - added by westonruter 8 years ago.
37890.3.diff (4.0 KB) - added by westonruter 8 years ago.

Download all attachments as: .zip

Change History (10)

8 years ago

#1 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.7

8 years ago

#2 @westonruter
8 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

@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.)

#3 @westonruter
8 years ago

  • Keywords has-patch added

#4 @dlh
8 years ago

@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.

8 years ago

#5 @westonruter
8 years ago

@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.

#6 @dlh
8 years ago

37890.3.diff also looks good. Thanks @westonruter!

#7 @westonruter
8 years ago

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

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.

Note: See TracTickets for help on using tickets.