Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
gwharton opened this issue Aug 29, 2024 · 5 comments
Open

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

gwharton opened this issue Aug 29, 2024 · 5 comments
Assignees
@gwharton
Labels
bug Something isn't working Progress: PR created

Comments

@gwharton
Copy link

gwharton commented Aug 29, 2024 •
edited
Loading

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.
@gwharton gwharton added the bug Something isn't working label Aug 29, 2024
@m2-assistant M2 Assistant
Copy link

m2-assistant bot commented 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
Copy link
Author

gwharton commented Aug 29, 2024 •
edited
Loading

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
Copy link
Author

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
Copy link
Author

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 gwharton mentioned this issue Aug 29, 2024
Open
@gwharton
Copy link
Author

@magento I am working on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gwharton gwharton

Labels
bug Something isn't working Progress: PR created
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@gwharton