WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#28436 closed defect (bug) (fixed)

allowed_themes filter does not pass blog_id

Reported by: rmccue Owned by: jeremyfelt
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.4
Component: Themes Keywords: good-first-bug has-patch
Focuses: multisite Cc:

Description

The allowed_themes does not pass the $blog_id parameter into the filter, so you can't currently filter based on the site.

(There's also no filters in WP_Theme::get_allowed_on_network or WP_Theme::get_allowed_on_site, plus the two arrays are combined together, so a filter just in WP_Theme::get_allowed_on_site would be inadequate for blacklisting themes for certain sites.)

Attachments (7)

29536.diff (498 bytes) - added by pauldewouters 4 years ago.
28436.diff (1.7 KB) - added by pauldewouters 4 years ago.
screenshot.png (66.2 KB) - added by posungucci 4 years ago.
screenshot
28436.2.diff (2.8 KB) - added by lamosty 4 years ago.
Pass $blog_id parameter into the filter and create unit test.
28436.3.diff (3.0 KB) - added by lamosty 4 years ago.
Resolved issues mentioned by @pento.
28436.4.diff (5.3 KB) - added by jeremyfelt 4 years ago.
28436.5.diff (6.0 KB) - added by jeremyfelt 4 years ago.

Download all attachments as: .zip

Change History (30)

#1 @DrewAPicture
5 years ago

Would you care to create a patch for the former? :-)

#2 follow-up: @nacin
5 years ago

I'd have to go back into the ticket and older code to jog my memory on this, but the current 'allowed_themes' filter is likely explicitly applied to the network list of themes only, for backwards compatibility reasons. (It's an old filter.) Expanding it to what is allowed on a site could be problematic, depending on how people are using it.

Off the top of my head, we should probably shift the 'allowed_themes' filter into ::get_allowed_on_network() and add a new filter to get_allowed().

#3 in reply to: ↑ 2 @rmccue
5 years ago

Replying to nacin:

I'd have to go back into the ticket and older code to jog my memory on this, but the current 'allowed_themes' filter is likely explicitly applied to the network list of themes only, for backwards compatibility reasons. (It's an old filter.)

Indeed, @since MU

Off the top of my head, we should probably shift the 'allowed_themes' filter into ::get_allowed_on_network() and add a new filter to get_allowed().

Agreed. :) We should add a filter to get_allowed_on_site as well as one for the combined value from get_allowed.

#4 @jeremyfelt
5 years ago

  • Focuses multisite added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

I would enjoy this filter.

#5 @jeremyfelt
4 years ago

  • Keywords good-first-bug added

Naming may get a bit confusing here as allowed_themes moves to get_allowed_on_network() and something new is needed for get_allowed() and get_allowed_on_site(). It all makes sense though.

@pauldewouters
4 years ago

@pauldewouters
4 years ago

#6 follow-up: @pauldewouters
4 years ago

Added a patch adding the filters. Also uploaded wrong file by mistake. How do I delete it?

#7 in reply to: ↑ 6 ; follow-up: @DrewAPicture
4 years ago

Replying to pauldewouters:

Added a patch adding the filters. Also uploaded wrong file by mistake. How do I delete it?

It's alright, just upload your new patch.

@posungucci
4 years ago

screenshot

#8 in reply to: ↑ 7 @pauldewouters
4 years ago

Replying to DrewAPicture:

Replying to pauldewouters:

Added a patch adding the filters. Also uploaded wrong file by mistake. How do I delete it?

It's alright, just upload your new patch.

The correct patch is 28436.diff

#9 @jeremyfelt
4 years ago

More background in #21099 and [21686]

@lamosty
4 years ago

Pass $blog_id parameter into the filter and create unit test.

#10 @lamosty
4 years ago

We are having a Contributors Day in Slovakia. As a team, we worked on a simple unit test for this ticket.

#11 @pento
4 years ago

  • Keywords has-patch added; needs-patch removed

Nice! There are a few code style and comments that need tweaking:

  • The docs for the allowed_themes_combined and allowed_themes_site filters don't include $blog_id.
  • The @since for the allowed_themes filter should stay as "MU", not be updated to "4.2" (we can set the other filter versions correctly when this patch is accepted into core).
  • In the unit tests, you're backing up the $wpdb->suppress_errors() state, but this doesn't appear to be changed anywhere.
  • Is there any particular reason for setting $_SERVER['REMOTE_ADDR'] in the unit tests?

For props purposes, would you mind listing who is on the team at the Contributor Day?

#12 @pento
4 years ago

Oh, and while I'm a big fan of closures, we do still need our unit tests to run in PHP 5.2. The unit test will need to run with a defined method.

@lamosty
4 years ago

Resolved issues mentioned by @pento.

#13 @lamosty
4 years ago

@pento thanks for the feedback, really appreciate that.

Point 3 and 4: I (carelessly) copied another unit test to create this one. Sorry.

List of the team (in no particular order):

  • michalzuber
  • dmsnell
  • johnnypea
  • rob

#14 @fliespl
4 years ago

#32371 was marked as a duplicate.

#15 @obenland
4 years ago

  • Owner set to lamosty 7
  • Status changed from new to assigned

#16 @swissspidy
4 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 4.5

Current patch looks good, just needs a small update:

  • Needs to use @since 4.5.0 in the DocBlock (trailing zero)
  • 'allowed_themes' filter is @since MU
  • setUp() / tearDown() in the test class are not needed when they just call the parent's method

This ticket was mentioned in Slack in #core-multisite by toscho. View the logs.


4 years ago

#18 @jeremyfelt
4 years ago

  • Keywords needs-refresh removed
  • Owner changed from lamosty 7 to jeremyfelt
  • Status changed from assigned to reviewing

#19 @jeremyfelt
4 years ago

In 36350:

Themes: Add initial tests for the allowed_themes filter.

We'll be adjusting the placement of this filter and adding two other related filters, so we should make sure it continues to work as expected after the change.

See #28436.

@jeremyfelt
4 years ago

#20 @jeremyfelt
4 years ago

Almost. :)

Any naming ideas for allowed_themes_combined? I'd like something a little less confusing.

We could use allowed_themes_network, but that may be confusing as allowed_themes will be the legacy filter in WP_Theme::get_allowed_on_network().

Other adjustments in 28436.4.diff:

  • There are two return points for the allowed_themes_site filter in WP_Theme::get_allowed_on_site().
  • The allowed_themes filter needs to be on the return of WP_Theme::get_allowed_on_network() rather than inside the isset() check, otherwise it will not always fire.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


4 years ago

@jeremyfelt
4 years ago

#22 @jeremyfelt
4 years ago

In 28436.5.diff using these:

  • allowed_themes is legacy and used to filter network allowed themes directly via WP_Theme::get_allowed_on_network().
  • network_allowed_themes is new in WP_Theme::get_allowed() and filters what is received from WP_Theme::get_allowed_on_network() before combining that result with site results.
  • site_allowed_themes can be used to filter site allowed themes via WP_Theme::get_allowed_on_site(). These results are combined with network allowed themes.

#23 @jeremyfelt
4 years ago

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

In 36366:

Themes: Enhance filtering options for allowed themes on a network.

  • Move the legacy allowed_themes filter to WP_Theme::get_allowed_on_network(), where it will continue to filter themes allowed on the network.
  • Add network_allowed_themes filter to WP_Theme::get_allowed() and pass $blog_id to provide context.
  • Add site_allowed_themes filter to WP_Theme::get_allowed_on_site() and pass $blog_id to provide context.

Props pauldewouters, lamosty, michalzuber, dmsnell, johnnypea, rob.
Fixes #28436.

Note: See TracTickets for help on using tickets.