WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 years ago

Last modified 17 months ago

#27583 closed enhancement (fixed)

Move color sanitize function to wp-includes/formatting.php

Reported by: tollmanz Owned by: westonruter
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.4
Component: Formatting Keywords: has-patch commit
Focuses: template Cc:

Description

sanitize_hex_color(), sanitize_hex_color_no_hash(), and maybe_hash_hex_color() are excellent functions for sanitizing color values. Unfortunately, they are defined in wp-includes/class-wp-customize-manager.php which is only included when the customizer is initialized. I recommend that the functions be moved to wp-includes/formatting.php, where other similar functions are located, so that they can be used everywhere in WordPress.

As an example, if you use a color input for adding a color option in a theme or plugin, you should be properly sanitizing/escaping the value when output in the HTML. Having these three functions available makes this process easy and consistent. Additionally, it provides a level of consistency between projects as different developers will be using the same function for sanitizing data.

Attachments (4)

27583.diff (5.3 KB) - added by downstairsdev 4 years ago.
Patch moves color sanitization functions into wp-includes/formatting.php.
27583.1.diff (3.6 KB) - added by tollmanz 4 years ago.
Focus the patch on only moving the functions to formatting.php
27583.2.diff (6.7 KB) - added by tollmanz 4 years ago.
Wrap sanitize_hex_color with is_hex_color
27583.3.diff (7.1 KB) - added by Dumian 3 years ago.
Patch by @tollmanz from ticket:27583#comment:14 with additional improvements suggested by @downstairsdev in ticket:27583#comment:16

Download all attachments as: .zip

Change History (28)

@downstairsdev
4 years ago

Patch moves color sanitization functions into wp-includes/formatting.php.

#1 follow-up: @downstairsdev
4 years ago

  • Keywords has-patch needs-testing added

There are many use cases for "sanitize_hex_color", "sanitize_hex_color_no_hash", and "maybe_hash_hex_color".

a) These core functions would make sanitization easier for developers adding their own options panel. I have use case in one of my own plugins that would simplify the of_validate_hex function (https://github.com/devinsays/options-framework-plugin/blob/master/includes/class-options-sanitization.php).

b) Sanitizing these color values on output (such as an inline style) would be simplified. See this ticket for the _s theme: https://github.com/Automattic/_s/issues/447.

c) If the meta data project ever gets adopted into core, color sanitization functions would need to be available globally (https://github.com/wordpress-metadata/metadata-ui-api).

d) It makes color sanitization more consistent throughout core (color options in background and header), and simplifies the code in those places.


I attached a patch (27583.diff) that moves the color sanitization functions out of 'wp-includes/class-wp-customize-manager.php' and into 'wp-includes/formatting.php'. This patch also simplifies the color sanitization in 'wp-admin/custom-background.php' and 'wp-admin/custom-header.php' by using the "sanitize_hex_color_no_hash" which is now available.

#2 follow-ups: @obenland
4 years ago

I think it would be nice if we could make them always return an empty string if sanitization was unsuccessful. Does the null return value serve any specific purpose?

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

Replying to obenland:

I think it would be nice if we could make them always return an empty string if sanitization was unsuccessful. Does the null return value serve any specific purpose?

I think that this is something that should be addressed in another ticket. I think it is a valid point and something to think about, but it does not relate directly to the issue at hand.

#4 in reply to: ↑ 1 @tollmanz
4 years ago

Replying to downstairsdev:

I attached a patch (27583.diff) that moves the color sanitization functions out of 'wp-includes/class-wp-customize-manager.php' and into 'wp-includes/formatting.php'. This patch also simplifies the color sanitization in 'wp-admin/custom-background.php' and 'wp-admin/custom-header.php' by using the "sanitize_hex_color_no_hash" which is now available.

I would argue that this is not appropriate for this ticket. I think after the function are moved into a universally accessible location, another ticket should be opened to implement the functions where they previously weren't available.

@tollmanz
4 years ago

Focus the patch on only moving the functions to formatting.php

#5 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov
4 years ago

Related: #19100

Replying to obenland:

I think it would be nice if we could make them always return an empty string if sanitization was unsuccessful. Does the null return value serve any specific purpose?

Looks like it does, see [20910].

#6 in reply to: ↑ 5 @obenland
4 years ago

Replying to SergeyBiryukov:

Looks like it does, see [20910].

_sanitize_header_textcolor() checks the return value of sanitize_hex_color_no_hash() with empty() though: Source.

#7 @downstairsdev
4 years ago

@tollmanz +1. Limiting scope to 27583.1.diff and moving the additional items into a new/alternate ticket sounds like a good route.

#8 @ocean90
4 years ago

  • Milestone changed from Awaiting Review to 4.0

So I would suggest this ticket for the move to wp-includes/formatting.php and another ticket for returning always a string as other sanitize_* functions are already doing.

This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.


4 years ago

#10 @miqrogroove
4 years ago

  • Keywords needs-testing removed

#11 @nacin
4 years ago

Per discussion with tollmanz and I think downstairsdev (both in person, and at separate times), I'd like to only move sanitize_hex_color(). There's limited benefit to moving the other two. Rather than having three global functions that almost do the same thing — with one clearly superior — let's just have one. The only reason for _no_hash() is back-compat with core stuff. A plugin or theme should avoid and move away from storing colors without a hash.

#12 follow-up: @nacin
4 years ago

  • Keywords 2nd-opinion added

Cancel that — sanitize_hex_color() is a misnomer. It doesn't sanitize anything. It validates or rejects.

We normally err on the side of sanitizing over validating in WordPress (of course, you could still validate using $unsanitized == sanitize( $unsanitized )), though in this case, it seems fine as-is. It just could benefit from a different name.

So, we could introduce validate_hex_color(), then leave sanitize_hex_color() where it is, and have it wrap it. Or offer a deprecation notice if we really wanted, as well.

#13 in reply to: ↑ 12 @jeremyfelt
4 years ago

Replying to nacin:

So, we could introduce validate_hex_color(), then leave sanitize_hex_color() where it is, and have it wrap it. Or offer a deprecation notice if we really wanted, as well.

Part of this is moving sanitize_hex_color() so that it's available outside of the customizer. We would need to move and wrap if we wanted both.

@tollmanz
4 years ago

Wrap sanitize_hex_color with is_hex_color

#14 @tollmanz
4 years ago

27583.2.diff is a first pass at integrating some of the ideas that @nacin and I discussed.

  • is_hex_color() has been added, which simply wraps sanitize_hex_color(). This is a validation function with a name that better suggests its function.
  • sanitize_hex_color() has been moved to wp-includes/formatting.php so it can be used by is_hex_color(), which is in the same file.
  • Changed use of sanitized_hex_color() in sanitize_hex_color_no_hash() to is_hex_color() to begin the migration to the correct function. This is the *only use of the function* in core, other than default themes.
  • I left out the _doing_it_wrong() notice at this point because I am not sure the best way to deprecate the function. Unfortunately, if we want to deprecate it, we will need to port the function's code to is_hex_color(). I am not sure the best route to handle this redundancy.
  • I added unit tests to cover is_hex_color(), sanitize_hex_color_no_hash(), and maybe_hash_hex_color().
  • I cleaned up docs where necessary and fixed a few code standard issues.

#15 @nacin
4 years ago

  • Milestone changed from 4.0 to Future Release

27583.2.diff looks good, but I still wonder about the name sanitize_hex_color(). It doesn't do sanitization. It does validation. Maybe I'm being overly pedantic, but what I think I would want to do is introduce validate_hex_color(), which returns either the hex color or false; have sanitize_hex_color() wrap it and probably deprecate that function; don't add an is_hex_color() as that's just a boolean cast of validate_hex_color(); keep sanitize_hex_color_no_hash() and maybe_hash_hex_color() where it is.

sanitize_hex_color_no_hash() and maybe_hash_hex_color() both do actually do what their names describe. That's a bit of a stretch for sanitize_hex_color_no_hash(), but it *does* remove a hash as appropriate.

Or we can just treat "sanitization" in this case as validation with a twist — rather than confirming whether it is OK, it will go ahead and return for you a "sanitized" color, and if it finds stuff it can't sanitize (as in, anything but that), it returns false. But that sounds like validation.

Since this function is so trivial (validating a hex color is tremendously easy; it's a single regex that will never need to be updated) I don't feel too bad about keeping this away from theme developers for a bit longer. Moving out of 4.0 for now as it is an enhancement. It could come back in easily with a consensus. If everyone disagrees with me or has good reason to think that WP has plenty of similar 'sanitize'-named functions that don't actually do that, etc., I'm easily sway-able.

#16 @downstairsdev
4 years ago

Perhaps sanitize_hex_color() could require a valid fallback. That's the only way I can think to return a valid sanitized hex that would be acceptable if the passed value doesn't validate. Similar to how sanitize_html_class works: https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/formatting.php#L1327.

This ticket was mentioned in Slack in #themereview by downstairsdev. View the logs.


4 years ago

#18 @ocean90
3 years ago

#31447 was marked as a duplicate.

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


3 years ago

This ticket was mentioned in Slack in #themereview by greenshady. View the logs.


3 years ago

@Dumian
3 years ago

Patch by @tollmanz from ticket:27583#comment:14 with additional improvements suggested by @downstairsdev in ticket:27583#comment:16

#21 @westonruter
2 years ago

  • Milestone changed from Future Release to 4.6
  • Owner set to westonruter
  • Status changed from new to accepted

I want to get these function definitions out of class-wp-customize-manager.php.

#22 @westonruter
2 years ago

  • Keywords commit added; 2nd-opinion removed

I'm going to go ahead with simply moving the functions out of class-wp-customize-manager.php and into formatting.php (27583.1.diff), since this was basically missed when #33413 broke out classes into their own separate PHP files.

The other improvements to the functions can be done in another ticket, as suggested above.

#23 @westonruter
2 years ago

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

In 37283:

Customize/Formatting: Move sanitize_hex_color(), sanitize_hex_color_no_hash(), and maybe_hash_hex_color() from class-wp-customize-manager.php into formatting.php.

Adds missing braces.

See #33413.
Props downstairsdev, tollmanz.
Fixes #27583.

This ticket was mentioned in Slack in #themereview by emiluzelac. View the logs.


17 months ago

Note: See TracTickets for help on using tickets.