WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 5 months ago

#53452 closed defect (bug) (fixed)

[WP 5.8 beta 2] $_POST['sidebar'] is missing when saving a widget

Reported by: strategio Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: REST API Keywords: has-patch has-unit-tests
Focuses: administration, rest-api Cc:

Description

It's a backward compatibility requirement because this data is available in the $_POST superglobal when running WP 5.7.2 and it can be used inside \WP_Widget::update.

Here's a simple example:

<?php
class My_Widget extends WP_Widget {

    public function update( $new_instance, $old_instance ) {
        $new_instance['sidebar'] = $_POST['sidebar'];

        return $new_instance;        
    }
}

I can see in \WP_REST_Widgets_Controller::save_widget that the $_POST superglobal is temporarily altered, so I assume we should be able to also include the 'sidebar'.

Change History (17)

#1 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 5.8

This ticket was mentioned in PR #1393 on WordPress/wordpress-develop by strategio.


6 months ago

  • Keywords has-patch added

#3 follow-up: @spacedmonkey
6 months ago

Why is this data needed? If it needed, then there is other data is missing. See this example.

This is example data.

widget-custom_html[2][title]: test
widget-custom_html[2][content]: Test!!
widget-id: custom_html-2
id_base: custom_html
widget-width: 400
widget-height: 350
widget_number: 2
multi_number:
add_new:
action: save-widget
savewidgets: 2d13e282bc
sidebar: sidebar-1

#4 in reply to: ↑ 3 @strategio
6 months ago

Replying to spacedmonkey:

Why is this data needed? If it needed, then there is other data is missing. See this example.

In our case, we have widgets that needs to know the assigned sidebar in order to set the initial settings. But there's no form element for our widget, the settings UI is shown in another place.

So to be honest, I didn't pay attention to the other form data, but I believe it should be passed to the superglobal to keep WP 5.8 backward compatible with legacy widgets.

For the 'sidebar', I believe it's legit to expose this information somewhere because \WP_Widget::update is called from \WP_REST_Widgets_Controller::save_widget which has this information as the second argument.

Last edited 6 months ago by strategio (previous) (diff)

#5 follow-up: @noisysocks
6 months ago

  • Component changed from Widgets to REST API
  • Keywords needs-unit-tests added

I'm OK with adding it for backwards compatibility. Could you please update the REST API controller's unit tests?

#6 in reply to: ↑ 5 @strategio
6 months ago

Replying to noisysocks:

I'm OK with adding it for backwards compatibility. Could you please update the REST API controller's unit tests?

Sure, I'll try to do this tomorrow (Friday).

#7 @noisysocks
6 months ago

  • Severity changed from critical to normal

#8 @desrosj
6 months ago

This is a pretty self contained change. I'm going to commit this without a unit test for the time being so that it can be included for testing in beta 4 later today.

@strategio We can add the test update when you have a chance to create the patch for it, but we should make sure to add it prior to RC1.

#9 @desrosj
6 months ago

In 51239:

REST API: Include the sidebar ID when saving a widget.

This ensures backwards compatibility for code expecting this field to be present.

Props strategio, noisysocks, spacedmonkey.
See #53452.

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


5 months ago

This ticket was mentioned in PR #1448 on WordPress/wordpress-develop by strategio.


5 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#13 @strategio
5 months ago

Hi @desrosj,

I added a test in https://github.com/WordPress/wordpress-develop/pull/1448. Please review it!

#14 @desrosj
5 months ago

Thanks @strategio! The test looks good to me, though I'm not sure about where the test class actually belongs.

It looks like we have a file in DIR_TESTDATA with some widgets, but there are only 2 and they are all tied to a specific data provider. I've also been trying to think of a way to refactor out the need for the class at all, but nothing comes to mind right now.

@SergeyBiryukov would you happen to have any suggestions here?

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


5 months ago

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


5 months ago

#17 @JeffPaul
5 months ago

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

Closing in favor of picking up the unit test for this in #53673. @strategio please feel free to drop your patch/PR on the subsequent ticket, though I did reference your comment in the description there.

Note: See TracTickets for help on using tickets.