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

[tollmanz]

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.

[downstairsdev]

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

[downstairsdev]

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.

[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?

[tollmanz]

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.

[tollmanz]

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]

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

[SergeyBiryukov]

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].

[obenland]

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.

[downstairsdev]

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

[ocean90]

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.

[nacin]

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.

[nacin]

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.

[jeremyfelt]

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]

Wrap sanitize_hex_color with is_hex_color

[tollmanz]

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.

[nacin]

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.

[downstairsdev]

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.

[ocean90]

#31447 was marked as a duplicate.

[Dumian]

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

[westonruter]

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

[westonruter]

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.

[westonruter]

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.