Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#45029 closed defect (bug) (fixed)

Changes by widget_{$this->id_base}_instance_schema are overridden by sub class.

Reported by: toro_unit's profile Toro_Unit Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9
Component: Widgets Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description

WP_Widget_Media::get_instance_schema has "widget_{$this->id_base}_instance_schema" filter.

  • WP_Widget_Media_Image::get_instance_schema
  • WP_Widget_Media_Video::get_instance_schema
  • WP_Widget_Media_Audio::get_instance_schema

are using array_marge. like this.

$schema = array_merge(
                        parent::get_instance_schema(),
                        array(
                                // setting for widgets.
                        )
                );

So, Change by the filter, if same key exist, overwritten.

Attachments (2)

45029.patch (7.3 KB) - added by Toro_Unit 6 years ago.
45029-2.diff (8.5 KB) - added by birgire 6 years ago.

Download all attachments as: .zip

Change History (17)

@Toro_Unit
6 years ago

#1 @Toro_Unit
6 years ago

  • Keywords has-patch has-unit-tests added

#2 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.0.1
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#3 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#4 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#5 @audrasjb
6 years ago

  • Keywords dev-feedback added

This ticket is triaged in milestone 5.0.3. Now it will needs commit and backport to the related branch. Adding dev-feedback keywords to see if it can land in 5.0.3 at the very beginning of January.

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


6 years ago

#7 @desrosj
6 years ago

  • Milestone changed from 5.0.3 to 5.1

Going to punt this to 5.1. It is outside of the scope for 5.0.3 (block
editor bugs, regressions, and major bugs).

#8 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

Patch needs review and a decision.

@birgire
6 years ago

#9 follow-up: @birgire
6 years ago

The patch in 45029.patch applies cleanly on my test install and the widgets tests run successfully.

The patch seems to correctly reverse the array_merge( $some_options, $defaults ) to array_merge( $defaults, $some_options ).

We can e.g. compare it with wp_parse_args( $options, $defaults ), that's a wrapper of array_merge( $defaults, $options ), regarding the expected input order.

In 45029-2.diff I added the filtering into it's own test method, with some minor adjustments, like:

  • Adding a ticket reference.
  • Adding some extra commenting on what the default values are that are going to be overridden by the filtering.
  • Adding @since on the filter callbacks.
  • Uses assertTrue( ... ) instead of assertSame( true, ... ).
Last edited 6 years ago by birgire (previous) (diff)

#10 in reply to: ↑ 9 ; follow-up: @SergeyBiryukov
6 years ago

Replying to birgire:

We can e.g. compare it with wp_parse_args( $options, $defaults ), that's a wrapper of array_merge( $defaults, $options ), regarding the expected input order.

Could we use wp_parse_args() here as well, for consistency with the rest of core? That should prevent further confusion.

#11 in reply to: ↑ 10 @birgire
6 years ago

Replying to SergeyBiryukov:

Replying to birgire:

We can e.g. compare it with wp_parse_args( $options, $defaults ), that's a wrapper of array_merge( $defaults, $options ), regarding the expected input order.

Could we use wp_parse_args() here as well, for consistency with the rest of core? That should prevent further confusion.

yes I think that would be possible, though we would also add support for string and object type of arguments.

As the use of wp_parse_args raises some other questions (see below), I would suggest we use the patch 45029-2.diff as is, until it's more clear what to do with the type hinting of @param :-)


I had a look at some wp_parse_args core uses and it seems there's no specific rule in place regarding how to document the input type; sometimes the type is documented as an array, but sometimes as array|string. It seems the object support is never documented, apart from the array|string|object input type of the wp_parse_args function.

This raises the question, how faithful the type-hinting for @param is in general to the actual supported types.

If I recall correctly, the general aim is to support array input arguments, rather than string parse the input.

If we change it to wp_parse_args, wouldn't we also modify inline documentation for the filter unchanged:

https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/widgets/class-wp-widget-media.php#L156

But it would be nice to have a guideline on this matter in the Handbook:

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/


ps: The inline documentation of wp_parse_args says:

https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/functions.php#L3943

This function is used throughout WordPress to allow for both string or array to be merged into another array.

but I think we should consider updating it as well (another ticket) to mention the object support.

Version 0, edited 6 years ago by birgire (next)

#13 @SergeyBiryukov
6 years ago

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

In 45100:

Widgets: Make sure changes to media widgets' instance schema via widget_{$this->id_base}_instance_schema filter are not overridden by subclasses.

Previously, WP_Widget_Media_Audio, WP_Widget_Media_Image, and WP_Widget_Media_Video used to override the changes due to reversed arguments in array_merge() call.

Props Toro_Unit, birgire.
Fixes #45029.

#14 @SergeyBiryukov
6 years ago

In 45101:

Widgets: Make sure changes to media widgets' instance schema via widget_{$this->id_base}_instance_schema filter are not overridden by subclasses.

Add unit tests missed in [45100].

Props Toro_Unit, birgire.
See #45029.

#15 @SergeyBiryukov
6 years ago

In 45103:

Docs: Remove an empty line between @param and @return in the tests added in [45101], per documentation coding standards.

See #45029.

Note: See TracTickets for help on using tickets.