Update escaping function #665
Conversation
| if ( | ||
| str_starts_with( $string, '<?php echo' ) || | ||
| str_starts_with( $string, '<?php esc_html_e' ) || | ||
| str_starts_with( $string, '<?php esc_html' ) | ||
| ) { | ||
| return $string; | ||
| } |
There was a problem hiding this comment.
There are plenty other escaping functions that might show up. I'm wondering if we could instead try to catch them all with regex:
| if ( | |
| str_starts_with( $string, '<?php echo' ) || | |
| str_starts_with( $string, '<?php esc_html_e' ) || | |
| str_starts_with( $string, '<?php esc_html' ) | |
| ) { | |
| return $string; | |
| } | |
| if ( | |
| preg_match( | |
| '/^<\?php\s+(echo|esc_)/', | |
| $string | |
| ) | |
| ) { | |
| return $string; | |
| } |
All current escaping functions match that pattern, but we could also just add them all to avoid catching unintended functions:
if (
preg_match(
'/^<\?php\s+(echo|esc_html_e|esc_html__|esc_html_x|esc_attr_e|esc_attr__|esc_attr_x|esc_url|esc_js|esc_textarea)/',
$string
)
) {
return $string;
}There was a problem hiding this comment.
Good point but I think we could simplify it by just checking if the string starts with <?php. That will cover all the cases in a simpler way. Implemented it here: 9b223b7
| @@ -13,29 +13,29 @@ class CBT_Theme_Locale_EscapeString extends CBT_Theme_Locale_UnitTestCase { | |||
| public function test_escape_string() { | |||
| $string = 'This is a test text.'; | |||
| $escaped_string = CBT_Theme_Locale::escape_string( $string ); | |||
| $this->assertEquals( "<?php echo __('This is a test text.', 'test-locale-theme');?>", $escaped_string ); | |||
| $this->assertEquals( "<?php esc_html_e('This is a test text.', 'test-locale-theme');?>", $escaped_string ); | |||
There was a problem hiding this comment.
Unrelated to the changes, but why are we using CamelCase for these test files? It'd be better if we kept the convention we're already using for the other files. Same for the directory name.
There was a problem hiding this comment.
That's the WordPress core convention I think we should follow it. Example: https://github.com/WordPress/wordpress-develop/tree/trunk/tests/phpunit/tests/blocks
There was a problem hiding this comment.
if we kept the convention we're already using for the other files. Same for the directory name.
Those should be progressively updated when we have the chance.
There was a problem hiding this comment.
I submitted an issue for standardizing the convention used on tests: #666
There was a problem hiding this comment.
I don't think those tests follow the actual convention either:
https://make.wordpress.org/docs/style-guide/formatting/filenames/#naming-files
Use lowercase file, folder, and directory names. In general, separate words in filenames with hyphens, not underscores. Use standard ASCII alphanumeric characters in file, folder, and directory names.
There was a problem hiding this comment.
Use lowercase file, folder, and directory names.
I'm not sure because, as far as I saw, the are numerous core test files using camel case.
Example 2: https://github.com/WordPress/wordpress-develop/tree/trunk/tests/phpunit/tests/blocks
Example 1: https://github.com/WordPress/wordpress-develop/tree/trunk/tests/phpunit/tests/fonts/font-face
What?
Replace a simple translate function call with an escaping and translating function when escaping is missing.
Why?
Fixes: #590