Update /theme-check/checks/class-title-check.php, replace reg pattern… #462
Conversation
… modifier (/s) with (/i)
There was a problem hiding this comment.
I agree with the /i addition, but the removal of the /s prevents the regex match multi-line svgs. Using both we can match uppercase and multiline simultaneously.
s (PCRE_DOTALL)
If this modifier is set, a dot metacharacter in the pattern matches all characters, including newlines. Without it, newlines are excluded. This modifier is equivalent to Perl's /s modifier. A negative class such as [^a] always matches a newline character, independent of the setting of this modifier.
checks/class-title-check.php
Outdated
| @@ -50,7 +50,7 @@ public function check( $php_files, $css_files, $other_files ) { | |||
| } | |||
|
|
|||
| // Look for anything that looks like <svg>...</svg> and exclude it (inline svg's have titles too). | |||
| $file_content = preg_replace( '/<svg.*>.*<\/svg>/s', '', $file_content ); | |||
| $file_content = preg_replace( '/<svg.*>.*<\/svg>/i', '', $file_content ); | |||
There was a problem hiding this comment.
I think we should use the 2 flags i and s.
| $file_content = preg_replace( '/<svg.*>.*<\/svg>/i', '', $file_content ); | |
| $file_content = preg_replace( '/<svg.*>.*<\/svg>/is', '', $file_content ); |
There was a problem hiding this comment.
Hi @matiasbenedetto,
Upon further reflection, I agree with your suggestion regarding the need for the 's' modifier. However, I discovered that using 's' on its own does not work since it nullifies the result of the modified line.
After a second round of troubleshooting, including refactoring the regex and the complete logic, I found that combining 's' with the 'U' modifier seems to be the most universal and effective solution.
I have updated my commit accordingly to reflect this change.
Combine the three 'isU' modifiers because 's' returns null and only behaves correctly when combined with 'U'. Additionally, including the case-sensitive 'i' modifier can potentially improve accuracy.
|
I tried the current code from the I tried the plugin with this file (note that the <?php
// PHP logic at the top, mixed with HTML
if (isset($_POST['submit'])) {
$name = $_POST['name'];
$color = $_POST['color'];
$size = isset($_POST['size']) ? $_POST['size'] : 'medium';
echo "<style>body { background-color: $color; }</style>";
}
?>
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<style>
.header {
font-size: 24px;
color: <?php echo isset($name) ? 'blue' : 'red'; ?>;
}
.container {
display: <?php if ($size == 'large') { echo 'flex'; } else { echo 'block'; } ?>;
}
.box {
width: 100px;
height: 100px;
background: yellow;
<?php if ($size == 'small') { echo 'width: 50px; height: 50px;'; } ?>
}
</style>
</head>
<body>
<div class="header"><?php echo isset($name) ? "Hello, $name" : "Welcome to the site"; ?></div>
<form method="post">
Name: <input type="text" name="name" value="<?php echo isset($name) ? $name : ''; ?>"><br>
Favorite Color: <input type="text" name="color" value="<?php echo isset($color) ? $color : ''; ?>"><br>
Size:
<input type="radio" name="size" value="small" <?php echo $size == 'small' ? 'checked' : ''; ?>> Small
<input type="radio" name="size" value="medium" <?php echo $size == 'medium' ? 'checked' : ''; ?>> Medium
<input type="radio" name="size" value="large" <?php echo $size == 'large' ? 'checked' : ''; ?>> Large
<br><br>
<input type="submit" name="submit" value="Submit">
</form>
<div class="container">
<?php
// More PHP mixed with HTML
if (isset($name)) {
echo "<div class='box'>Box for $name</div>";
} else {
echo "<div class='box'>Default Box</div>";
}
?>
<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">
<title>Random Square</title>
<rect width="100" height="100" style="fill:<?php echo isset($color) ? $color : 'black'; ?>;" />
</svg>
</div>
<?php
// Closing out with PHP again
if (!isset($name)) {
echo "<p>Enter your details above to see changes!</p>";
}
?>
</body>
</html> |
|
Hi @matiasbenedetto, For ease of reproducing the bug, I recommend using the 'Transact' theme to verify the bug. Transact, a child theme, doesn't include 'svg', but its parent theme, Productive eCommerce, does. When using the Theme Check plugin, the error is triggered in Productive eCommerce, specifically in this file: /productive-ecommerce/admin/common/options/global/global-settings-admin.php To reproduce the error, please:
You should encounter the same error raised in the ticket link above, originally detected by @kafleg. Update on the Fix I hope this provides further clarity on the bug's history and the proposed fix. Please let me know if you need any additional information. |
|
Hi @matiasbenedetto <https://github.com/matiasbenedetto>,
The bug was initially reported here:
https://themes.trac.wordpress.org/ticket/184086#comment:21
For ease of reproducing the bug, I recommend using the 'Transact' theme to
verify the bug. Transact, a child theme, doesn't include 'svg', but its
parent theme, Productive eCommerce, does.
When using the Theme Check plugin, the error is triggered in Productive
eCommerce, specifically in this file:
/productive-ecommerce/admin/common/options/global/global-settings-admin.php
*To reproduce the error, please:*
1. Download the Transact theme and its parent theme
2. Test Transact using the Theme Check plugin
You should encounter the same error raised in the ticket link above,
originally detected by @kafleg <https://github.com/kafleg>.
*Update on the Fix*
The error has now been resolved, as described in my PR. I found that adding
two other modifiers to support the initial 's' modifier resolved the issue.
I hope this provides further clarity on the bug's history and the proposed
fix. Please let me know if you need any additional information.
Cheers
…On Thu, Oct 10, 2024 at 5:46 PM Matias Benedetto ***@***.***> wrote:
I tried the current code from the master branch, and it seems to be
working fine. I'm not sure what problem this PR is fixing. Could you add
testing instructions and edit the title of the PR to be clearer about
functionality?
I tried the plugin with this file (note that the svg tag has a <title>
inside. And the check is working as expected (not raising an error).
<?php// PHP logic at the top, mixed with HTMLif (isset($_POST['submit'])) {
$name = $_POST['name'];
$color = $_POST['color'];
$size = isset($_POST['size']) ? $_POST['size'] : 'medium';
echo "<style>body { background-color: $color; }</style>";
}?>
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<style>
.header {
font-size: 24px;
color: <?php echo isset($name) ? 'blue' : 'red'; ?>;
}
.container {
display: <?php if ($size == 'large') { echo 'flex'; } else { echo 'block'; } ?>;
}
.box {
width: 100px;
height: 100px;
background: yellow;
<?php if ($size == 'small') { echo 'width: 50px; height: 50px;'; } ?>
}
</style>
</head>
<body>
<div class="header"><?php echo isset($name) ? "Hello, $name" : "Welcome to the site"; ?></div>
<form method="post">
Name: <input type="text" name="name" value="<?php echo isset($name) ? $name : ''; ?>"><br>
Favorite Color: <input type="text" name="color" value="<?php echo isset($color) ? $color : ''; ?>"><br>
Size:
<input type="radio" name="size" value="small" <?php echo $size == 'small' ? 'checked' : ''; ?>> Small
<input type="radio" name="size" value="medium" <?php echo $size == 'medium' ? 'checked' : ''; ?>> Medium
<input type="radio" name="size" value="large" <?php echo $size == 'large' ? 'checked' : ''; ?>> Large
<br><br>
<input type="submit" name="submit" value="Submit">
</form>
<div class="container">
<?php
// More PHP mixed with HTML
if (isset($name)) {
echo "<div class='box'>Box for $name</div>";
} else {
echo "<div class='box'>Default Box</div>";
}
?>
<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">
<title>Random Square</title>
<rect width="100" height="100" style="fill:<?php echo isset($color) ? $color : 'black'; ?>;" />
</svg>
</div>
<?php
// Closing out with PHP again
if (!isset($name)) {
echo "<p>Enter your details above to see changes!</p>";
}
?>
</body>
</html>
—
Reply to this email directly, view it on GitHub
<#462 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BLFWF4WBFYKDLTZ7JM6DIQ3Z22VORAVCNFSM6AAAAABOJNAJOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBVGU4TONJXGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
… modifier (/s) with (/i)
File: /theme-check/checks/class-title-check.php
Error:
Around line 57 of the file, an attempt is made to trigger an error if any file uses a variation of the word "title" in an unexpected way. SVG files are intended to be exempted from this check. However, due to the use of the "/s" modifier in the regular expression on line 53, the content of any file containing SVG tags is treated as a single line, effectively nullifying the file content.
This causes an error on line 57, where the strpos() function does not accept null as a parameter.
Fix:
The fix replaces the "/s" modifier with "/i" in the regular expression. The "/i" modifier makes the pattern case-insensitive, which is more appropriate for this check.
Further information:
Please see the full list of allowed modifiers for your review: https://www.php.net/manual/en/reference.pcre.pattern.modifiers.php