-
Notifications
You must be signed in to change notification settings - Fork 162
Description
Preconditions
- Latest Magento Coding Standards Package. Reproduced on version 33.
Steps to reproduce
The following creates a blank project, installs the necessary composer packages, creates some test files, and then patches phpcs to provide additional debugging output on the console for regex matching of include paths.
mkdir testproject
cd testproject
composer require magento/magento-coding-standard
tee test.php >/dev/null <<'EOF'
<?php
class Test extends ParentClassTest
{
public function __construct() {
parent::__construct();
}
}
EOF
tee test.xml >/dev/null <<'EOF'
<?xml version="1.0" encoding="UTF-8" ?>
<root>
<test>Hello</test>
</root>
EOF
tee test.less >/dev/null <<'EOF'
.selector {
width: 0;
}
EOF
tee test.html >/dev/null <<'EOF'
<html>
<head>
</head>
<body>
</body>
</html>
EOF
tee test.phtml >/dev/null <<'EOF'
<p>Some Paragraph Text</p>
<?php
$test="Test";
echo $escaper->escapeHtml($test);
?>
EOF
mkdir -p anything/view/adminhtml/anything
mkdir -p anything/etc
mkdir -p etc/anything
mkdir -p etc/adminhtml
cp -f test.xml anything/view/adminhtml/anything/test.xml
cp -f test.xml anything/di.xml
cp -f test.xml anything/module.xml
cp -f test.xml anything/widget.xml
cp -f test.xml etc/config.xml
cp -f test.xml etc/config.anything.xml
cp -f test.xml etc/anything/config.xml
cp -f test.phtml anything/test.phtml
cp -f test.xml anything/etc/test.xml
cp -f test.xml etc/adminhtml/system.xml
cp -f test.xml etc/config.xml
cp -f test.xml etc/anything/config.xml
cp -f test.xml etc/config.xml
cp -f test.xml etc/config.anything.xml
composer require vaimo/composer-patches
composer config extra.patches-search patches
mkdir patches
tee patches/additional-debug-output.patch >/dev/null <<'EOF'
@package squizlabs/php_codesniffer
--- src/Files/File.original.php 2024-08-29 10:37:43.349529594 +0100
+++ src/Files/File.php 2024-08-29 10:39:12.000000000 +0100
@@ -319,6 +319,7 @@
*/
public function process()
{
+ echo "GWDEBUG: Path : " . $this->path . "\n";
if ($this->ignored === true) {
return;
}
@@ -493,7 +494,10 @@
}
$pattern = '`'.$pattern.'`i';
+ echo "GWDEBUG: Class : " . $class . "\n";
+ echo "GWDEBUG: Pattern : " . $pattern . "\n";
if (preg_match($pattern, $this->path) === 1) {
+ echo "GWDEBUG: MATCHED\n";
$included = true;
break;
}
EOF
composer update
I went through all of the Magento sniffs that have an include-path regex specified in the ruleset.xml file and verified that matches were being made where expected. I did this both on Ubuntu 24.04 and on Windows 11/Cygwin. Here is a summary of the tests that dd not produce the expected matching behaviour and some notes.
Ubuntu 24.04 - Magento2.Html.HtmlBinding
PHPCS Magento Ruleset Include Pattern : *\/.phtml$
ubuntu@megumi:~/testproject$ vendor/bin/phpcs --sniffs=Magento2.Html.HtmlBinding --standard=Magento2 test.phtml
GWDEBUG: Path : /home/ubuntu/testproject/test.phtml
GWDEBUG: Class : Magento2\Sniffs\Html\HtmlBindingSniff
GWDEBUG: Pattern : `.*\/.phtml$`i
ubuntu@megumi:~/testproject$
NO MATCH
I am assuming that this test is intended to run on all files with a .phtml
extension. As written, the expression will match on path/.phtml
but not path/file.phtml
.
Ubuntu 24.04 - Magento2.Legacy.Layout
PHPCS Magento Ruleset Include Pattern : */view/(adminhtml|frontend|base)/*\/.xml
ubuntu@megumi:~/testproject$ vendor/bin/phpcs --sniffs=Magento2.Legacy.Layout --standard=Magento2 anything/view/adminhtml/anything/test.xml
GWDEBUG: Path : /home/ubuntu/testproject/anything/view/adminhtml/anything/test.xml
GWDEBUG: Class : Magento2\Sniffs\Legacy\LayoutSniff
GWDEBUG: Pattern : `.*/view/(adminhtml|frontend|base)/.*\/.xml`i
ubuntu@megumi:~/testproject$
NO MATCH
Looking at the pattern, I would assume that this is intended to match files with filenames such as anything/view/adminhtml/anything/test.xml
, but thats not what the expression is looking for. Is it really looking for an xml file with no name and just extension.
Windows 11/Cygwin - Magento2.Legacy.ClassReferencesInConfigurationFiles
PHPCS Magento Ruleset Include Pattern : *\/etc/*.xml$
graham@DESKTOP-IAGIISG ~/testproject
$ vendor/bin/phpcs --sniffs=Magento2.Legacy.ClassReferencesInConfigurationFiles --standard=Magento2 anything/etc/test.xml
GWDEBUG: Path : C:\Users\GrahamWharton\testproject\anything\etc\test.xml
GWDEBUG: Class : Magento2\Sniffs\Legacy\ClassReferencesInConfigurationFilesSniff
GWDEBUG: Pattern : `.*\\\etc\\.*.xml$`i
graham@DESKTOP-IAGIISG ~/testproject
NO MATCH
The pattern is wrong here. It is matching \\
(Backslash) and then \e
(ASCII 8) followed by tc\something.xml.
Windows 11/Cygwin - Magento2.Html.HtmlBinding
PHPCS Magento Ruleset Include Pattern : *\/.phtml$
graham@DESKTOP-IAGIISG ~/testproject
$ vendor/bin/phpcs --sniffs=Magento2.Html.HtmlBinding --standard=Magento2 test.phtml
GWDEBUG: Path : C:\Users\GrahamWharton\testproject\test.phtml
GWDEBUG: Class : Magento2\Sniffs\Html\HtmlBindingSniff
GWDEBUG: Pattern : `.*\\\.phtml$`i
graham@DESKTOP-IAGIISG ~/testproject
NO MATCH
The pattern is wrong here. It is matching \\
(Backslash) and then \.
(.
) and then phtml
. i.e filename ends in \.phtml
Windows 11/Cygwin - Magento2.Legacy.ModuleXML
PHPCS Magento Ruleset Include Pattern : *\/module.xml$
graham@DESKTOP-IAGIISG ~/testproject
$ vendor/bin/phpcs --sniffs=Magento2.Legacy.ModuleXML --standard=Magento2 anything/module.xml
GWDEBUG: Path : C:\Users\GrahamWharton\testproject\anything\module.xml
GWDEBUG: Class : Magento2\Sniffs\Legacy\ModuleXMLSniff
GWDEBUG: Pattern : `.*\\\module.xml$`i
graham@DESKTOP-IAGIISG ~/testproject
NO MATCH
The pattern is wrong here. It is matching \\
(Backslash) and then \m
(undefined, preg_match should throw an error here) and then odule.xml
.
Windows 11/Cygwin - Magento2.Legacy.DiConfig
PHPCS Magento Ruleset Include Pattern : *\/di.xml$
graham@DESKTOP-IAGIISG ~/testproject
$ vendor/bin/phpcs --sniffs=Magento2.Legacy.DiConfig --standard=Magento2 anything/di.xml
GWDEBUG: Path : C:\Users\GrahamWharton\testproject\anything\di.xml
GWDEBUG: Class : Magento2\Sniffs\Legacy\DiConfigSniff
GWDEBUG: Pattern : `.*\\\di.xml$`i
graham@DESKTOP-IAGIISG ~/testproject
NO MATCH
The pattern is wrong here. It is matching \\
(Backslash) and then \d
(a digit from [0-9]
) and then i.xml
.
Windows 11/Cygwin - Magento2.Legacy.WidgetXML
PHPCS Magento Ruleset Include Pattern : *\/widget.xml$
graham@DESKTOP-IAGIISG ~/testproject
$ vendor/bin/phpcs --sniffs=Magento2.Legacy.WidgetXML --standard=Magento2 anything/widget.xml
GWDEBUG: Path : C:\Users\GrahamWharton\testproject\anything\widget.xml
GWDEBUG: Class : Magento2\Sniffs\Legacy\WidgetXMLSniff
GWDEBUG: Pattern : `.*\\\widget.xml$`i
GWDEBUG: MATCHED
GWDEBUG: Class : Magento2\Sniffs\Legacy\WidgetXMLSniff
GWDEBUG: Pattern : `.*\\\widget.xml$`i
GWDEBUG: MATCHED
GWDEBUG: Class : Magento2\Sniffs\Legacy\WidgetXMLSniff
GWDEBUG: Pattern : `.*\\\widget.xml$`i
GWDEBUG: MATCHED
GWDEBUG: Class : Magento2\Sniffs\Legacy\WidgetXMLSniff
GWDEBUG: Pattern : `.*\\\widget.xml$`i
GWDEBUG: MATCHED
graham@DESKTOP-IAGIISG ~/testproject
MATCHED but.......
Well ok, the regex matches on this one, but not by design, but by accident. The pattern as written will match anything followed by \\
(Backslash) followed by \w
(and word character [a-zA-Z0-9_]
) followed by idget.xml
.
Activity
m2-assistant commentedon Aug 29, 2024
Hi @gwharton. Thank you for your report.
To speed up processing of this issue, make sure that you provided sufficient information.
Add a comment to assign the issue:
@magento I am working on this
Join Magento Community Engineering Slack and ask your questions in #github channel.
gwharton commentedon Aug 29, 2024
FYI, the full set of sniffs I tested for regex match was
gwharton commentedon Aug 29, 2024
You can also see evidence of the \m being present on windows in the regex where phpcs fails and bugs out because of the Magento2.Legacy.ModuleXML include regex being wrong on windows.
gwharton commentedon Aug 29, 2024
If you'd like me to package this test environment up onto packagist to make it easier for you to reproduce, just let me know. More than happy to help.
gwharton commentedon Aug 29, 2024
@magento I am working on this