#45029 closed defect (bug) (fixed)
Changes by widget_{$this->id_base}_instance_schema are overridden by sub class.
Reported by: | Toro_Unit | Owned by: | 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)
Change History (17)
#2
@
6 years ago
- Milestone changed from Awaiting Review to 5.0.1
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
#7
@
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).
#9
follow-up:
↓ 10
@
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 ofassertSame( true, ... )
.
#10
in reply to:
↑ 9
;
follow-up:
↓ 11
@
6 years ago
Replying to birgire:
We can e.g. compare it with
wp_parse_args( $options, $defaults )
, that's a wrapper ofarray_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
@
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 ofarray_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:
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.
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.