-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted an issue for standardizing the convention used on tests: #666
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What?
Replace a simple translate function call with an escaping and translating function when escaping is missing.
Why?
Fixes: #590