WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29079 closed defect (bug) (fixed)

themes_api_result filter should use original arguments

Reported by: Corphi Owned by: SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.7.1
Component: Themes Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

The Themes API currently behaves differently whether it was overridden or it used its native method of querying theme information.

If the API is overridden by a custom implementation, the $args argument to the filter is - as documented - the original $args object that was passed to the API.

If the API uses the native method, the $args variable gets overridden by an array of arguments to a HTTP query. This behavior is neither documented nor intuitive.

The attached patch separates these two concerns and makes the HTTP query arguments available as a further parameter to the filter. But to me, they only seem useful for debugging (where WP_Http seems the right place to hook into) and should probably be dropped entirely. I created a patch for this scenario, too.

Attachments (3)

http-args-separated.patch (2.6 KB) - added by Corphi 5 years ago.
Separate query args, use as additional filter parameter.
http-args-dropped.patch (1.2 KB) - added by Corphi 5 years ago.
Separate query args, don’t hand HTTP args to the filter.
29079.diff (2.4 KB) - added by obenland 5 years ago.

Download all attachments as: .zip

Change History (17)

@Corphi
5 years ago

Separate query args, use as additional filter parameter.

@Corphi
5 years ago

Separate query args, don’t hand HTTP args to the filter.

#1 @obenland
5 years ago

I agree, variable names could be a little more verbose to be less confusing.
http-args-dropped.patch seems like a good way to do that.

It looks like it was introduced in r25956, breaking backwards compatibility for the themes_api_result filter.
Fixing it would restore previous behavior, but would break compatibility with implementations since then. :(

#2 @obenland
5 years ago

  • Keywords has-patch dev-feedback added
  • Type changed from defect (bug) to enhancement
  • Version changed from 3.9.1 to 3.7.1

#3 @Corphi
5 years ago

  • Type changed from enhancement to defect (bug)

Problem persists through version 4.1.
By definition in the glossary, this is a bug (unexpected result).

#4 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.2

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


5 years ago

#6 @SergeyBiryukov
5 years ago

This applies to the plugins_api_result filter as well.

These filters were introduced in 2.7 and 2.8, so [25956] was relatively not too long ago.

http-args-dropped.patch seems like the way to go, perhaps with a post on make/core.

I'll do a quick search through the plugin repo to see if there are any affected plugins.

@obenland
5 years ago

#7 @obenland
5 years ago

  • Keywords dev-feedback removed

#8 @SergeyBiryukov
5 years ago

Results from the repository:

awesome-flickr-gallery-plugin/afg_update.php
caldera-forms/classes/selldock_updater.php
crm-by-tpc/TPC/Helper/AutoUpdate.php
free-guest-post/ghost_writer.php
kento-pricing-table-free/kento-pricing-table-free.php
layout-engine/themes.php
myblogu/myblogu.php
orbisius-member-only-downloads-for-s2member/orbisius-s2member-only-downloads.php
plugin-bundles/includes/bundles_overview.php
plugin-download-display/plugin-download-display.php
plugin-promoter/plugin-promoter.php
plugintel/plugintel.php
pricingtable/pricingtable.php
sendpress/classes/class-sendpress-pro-manager.php
social-deux/admin/includes/wp-updates-plugin.php
wp-autoloader/lib/WPExtend/BasicWPFilters.php
wp-plugin-installer/wp-plugin-installer.php
wpzing/includes/core.php

WP Plugin Installer would be fixed by this change, others appear to be unaffected.

Searching through themes now, just in case.

#9 @obenland
5 years ago

Nice! We could add those in the post and talk about it breaking that plugin.

#10 @SergeyBiryukov
5 years ago

  • Keywords commit added

It actually fixes that plugin :) (Assuming it still works otherwise, haven't tested.)

No matches for (plugins|themes)_api_result in the theme repository.

#11 follow-up: @Ipstenu
5 years ago

Thanks, @dd32, for alerting. Don't worry about the broken plugins. We'll handle it :)

#12 in reply to: ↑ 11 @dd32
5 years ago

Replying to Ipstenu:

Thanks, @dd32, for alerting. Don't worry about the broken plugins. We'll handle it :)

FYI for the ticket; Half of these plugins were including a custom update handler - which is a big no-no for the plugins directory. The other half only had http functions, or included update functionality for other things (a few offer extra repo's for themes, optional extras for install, or premium upgrade paths) which were fine.

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


5 years ago

#14 @SergeyBiryukov
5 years ago

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

In 31363:

Avoid inadvertent stomping of the original $args parameter passed to plugins_api_result and themes_api_result filters in plugins_api() and themes_api(), respectively.

Fixes a regression introduced in [25956].

props Corphi, obenland.
fixes #29079.

Note: See TracTickets for help on using tickets.