search

magento / magento-coding-standard

Magento Coding Standard
Open Software License 3.0
349 stars 153 forks source link

Several include-path regex issues on both Windows and Linux #485

Open gwharton opened 2 months ago

gwharton commented 2 months ago

Preconditions

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

m2-assistant[bot] commented 2 months ago

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 commented 2 months ago

FYI, the full set of sniffs I tested for regex match was

vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteConfigNodes --standard=Magento2 etc/config.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteConfigNodes --standard=Magento2 etc/config.anything.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteConfigNodes --standard=Magento2 etc/anything/config.xml
vendor/bin/phpcs --sniffs=Magento2.PHP.AutogeneratedClassNotInConstructor --standard=Magento2 test.php
vendor/bin/phpcs --sniffs=Magento2.Html.HtmlCollapsibleAttribute --standard=Magento2 test.html
vendor/bin/phpcs --sniffs=Magento2.Html.HtmlCollapsibleAttribute --standard=Magento2 test.phtml
vendor/bin/phpcs --sniffs=Magento2.Legacy.Layout --standard=Magento2 anything/view/adminhtml/anything/test.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ClassReferencesInConfigurationFiles --standard=Magento2 anything/etc/test.xml
vendor/bin/phpcs --sniffs=Magento2.Security.XssTemplate --standard=Magento2 test.phtml
vendor/bin/phpcs --sniffs=Magento2.Html.HtmlBinding --standard=Magento2 test.phtml
vendor/bin/phpcs --sniffs=Magento2.Templates.ThisInTemplate --standard=Magento2 test.phtml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ModuleXML --standard=Magento2 anything/module.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.DiConfig --standard=Magento2 anything/di.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.WidgetXML --standard=Magento2 anything/widget.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteAcl --standard=Magento2 etc/config.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteAcl --standard=Magento2 etc/config.anything.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteAcl --standard=Magento2 etc/anything/config.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteMenu --standard=Magento2 etc/config.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteMenu --standard=Magento2 etc/config.anything.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteMenu --standard=Magento2 etc/anything/config.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.ObsoleteSystemConfiguration --standard=Magento2 etc/adminhtml/system.xml
vendor/bin/phpcs --sniffs=Magento2.Legacy.PhtmlTemplate --standard=Magento2 test.phtml
vendor/bin/phpcs --sniffs=Magento2.Less.Indentation --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.TypeSelectors --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.ClassNaming --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.CommentLevels --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.PropertiesSorting --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.BracesFormatting --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.SemicolonSpacing --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.ColonSpacing --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.ZeroUnits --standard=Magento2 test.less
vendor/bin/phpcs --sniffs=Magento2.Less.PropertiesLineBreak --standard=Magento2 test.less
gwharton commented 2 months ago

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.

graham@DESKTOP-IAGIISG ~/testproject
$ vendor/bin/phpcs  --standard=Magento2 test.xml

FILE: C:\Users\GrahamWharton\testproject\test.xml
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: preg_match(): Compilation failed: unrecognized character follows \ at offset 5 in C:\Users\GrahamWharton\testproject\vendor\squizlabs\php_codesniffer\src\Files\File.php 
   |       | on line 496
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 239ms; Memory: 14MB

graham@DESKTOP-IAGIISG ~/testproject
$
gwharton commented 2 months ago

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 commented 2 months ago

@magento I am working on this