WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#35698 closed enhancement (duplicate)

Distinguish between regular (site) options and network options in `sanitize_option()`

Reported by: flixos90 Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Formatting Keywords:
Focuses: multisite Cc:

Description

Currently the filter in sanitize_option() runs for both "regular" site options and network options as well. I think the kind of option being sanitized should be distinguished since an option with the same slug might have a completely different meaning in the context of a site vs a network.

I'm proposing to add a new filter sanitize_network_option_{$option} which only runs when the option being sanitized is a network option. A new (internal) parameter could be added to the function itself to tell the function what kind of option is sanitized.

Besides fixing possible problems with site and network options of the same name, this might also open up for an implementation of #15691 (as in register_network_setting() or something similar).

Attachments (4)

35698.diff (2.3 KB) - added by flixos90 3 years ago.
patch adding the new filter and function parameter
35698.2.diff (3.0 KB) - added by flixos90 3 years ago.
35698.3.diff (2.8 KB) - added by flixos90 3 years ago.
change filter
35698.4.diff (2.8 KB) - added by flixos90 3 years ago.
changed filter name

Download all attachments as: .zip

Change History (22)

@flixos90
3 years ago

patch adding the new filter and function parameter

#1 follow-up: @flixos90
3 years ago

  • Keywords has-patch added

35698.diff adds the new filter and a $scope parameter which can be either site (default) or network to sanitize_option(). It also adjusts the function calls in add_network_option() and update_network_option().

For backwards compatibility reasons I kept the regular filter in sanitize_option() active for a network setting as well. Although I would prefer to only run it if $scope is site, I'm not sure we can do that without breaking something. Maybe we could deprecate the filter if a network option is sanitized although I don't think I have ever seen deprecated filters before...

#2 in reply to: ↑ 1 ; follow-up: @jeremyfelt
3 years ago

  • Milestone changed from Awaiting Review to Future Release

+1, also related: #35379, which overlaps with this ticket quite a bit.

Replying to flixos90:

For backwards compatibility reasons I kept the regular filter in sanitize_option() active for a network setting as well. Although I would prefer to only run it if $scope is site, I'm not sure we can do that without breaking something. Maybe we could deprecate the filter if a network option is sanitized although I don't think I have ever seen deprecated filters before...

Yeah, we'll need to leave the site filter there. If anyone is filtering that, hopefully they're also being smart about whether the option is site or network in scope.

#3 @jeremyfelt
3 years ago

  • Milestone changed from Future Release to 4.5

#4 in reply to: ↑ 2 @flixos90
3 years ago

Replying to jeremyfelt:

Yeah, we'll need to leave the site filter there. If anyone is filtering that, hopefully they're also being smart about whether the option is site or network in scope.

In that case maybe we could even add another new filter for site options that is only run for a regular option. Probably a lot of the filters that are attached to sanitize_option_{$option} are registered automatically by register_setting() - so maybe we could add a sanitize_site_option_{$option} filter, and then in register_setting() apply the callback to that new filter instead of the current one. This should reduce the probability of conflicts drastically.
It would also mean that the sanitize_option_{$option} would solely exist for the purpose of back compat, since we would replace it with the new filter in all areas of WordPress Core.

I'm not sure about the naming conventions however, since, although it is technically a site option, we always call those simply "option", and if the filter uses "site_option", it might add to the already existing confusion with blog - site - network. Still, I suppose functionality goes over weird naming, so I think it would be a good idea to add this other filter as well.

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


3 years ago

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


3 years ago

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


3 years ago

#8 @ericlewis
3 years ago

  • Milestone changed from 4.5 to Future Release

Punting this from the 4.5 milestone as we're dropping beta tomorrow.

#9 @jeremyfelt
3 years ago

Some of this may also be resolved via the bug ticket #35379. I'll swing back around in the next couple days and take a closer look.

@flixos90
3 years ago

#10 @flixos90
3 years ago

35698.2.diff is an updated patch with the sanitize_site_option_{$option} filter added, renamed $scope to $context and updated docs.

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


3 years ago

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


3 years ago

#13 @jeremyfelt
3 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Future Release to 4.6

#14 follow-up: @jeremyfelt
3 years ago

This is looking good, @flixos90.

I'm pausing a bit on the sanitize_site_option_ filter only because we still support the _site_option() functions and that may lead to some confusion.

Would it be strange to do something like sanitize_{$context}_context_option_{$option} to clarify that sanitization is happening in a specific context?

Another option may be to pass $context as another parameter to sanitize_option_{$option} so that it's available for those who need it. Core's use is probably more concerned with the actual $context parameter and how that determines where to fallback when an error is encountered.

@flixos90
3 years ago

change filter

@flixos90
3 years ago

changed filter name

#15 in reply to: ↑ 14 @flixos90
3 years ago

Replying to jeremyfelt:

Would it be strange to do something like sanitize_{$context}_context_option_{$option} to clarify that sanitization is happening in a specific context?

Agreed, using site_option in the filter name would confuse a bit since we (unfortunately) call network options this occasionally. :) In 35698.4.diff I applied this change.

Another option may be to pass $context as another parameter to sanitize_option_{$option} so that it's available for those who need it. Core's use is probably more concerned with the actual $context parameter and how that determines where to fallback when an error is encountered.

I prefer the solution of having the context in the filter name. If it was a parameter, we would do a lot of add_filter just to return directly when it doesn't have the context we expect. For example I could see register_setting() to be network compatible at some point, in which case it would use the new filter from here. If we just passed $context as a parameter, this would cause problems because we couldn't add a filter only to a regular vs network option.

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


3 years ago

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


3 years ago

#18 @flixos90
3 years ago

  • Keywords has-patch needs-unit-tests removed
  • Milestone 4.6 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #35379.

Note: See TracTickets for help on using tickets.