Merge pull request #37 from AmpersandHQ/fix-php-warning

Fix "Plugin detection with virtual types can give php warnings"
AmpersandHQ · Oct 13, 2020 · 4c02a71 · 4c02a71
2 parents 42dfff5 + 8cabda3
commit 4c02a71
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 3 deletions.
diff --git a/dev/TestModule/app/code/Ampersand/Test/Plugin/AdobeImsUserProfileVirtual.php b/dev/TestModule/app/code/Ampersand/Test/Plugin/AdobeImsUserProfileVirtual.php
@@ -0,0 +1,20 @@
+<?php
+namespace Ampersand\Test\Plugin;
+
+class AdobeImsUserProfileVirtual
+{
+    public function beforeGetUpdatedAt($subject)
+    {
+        // do stuff
+    }
+
+    public function afterGetUpdatedAt($subject, $result)
+    {
+        return $result;
+    }
+
+    public function aroundGetUpdatedAt($subject, callable $proceed)
+    {
+        return $proceed();
+    }
+}
diff --git a/dev/TestModule/app/code/Ampersand/Test/etc/di.xml b/dev/TestModule/app/code/Ampersand/Test/etc/di.xml
@@ -14,6 +14,7 @@
 
         <type name="Magento\AdobeIms\Model\UserProfile">
                 <plugin name="AmpersandTestPluginBeforeAfterAround3\UserProfile" type="Ampersand\Test\Plugin\AdobeImsUserProfile" sortOrder="1" />
+                <plugin name="somethingVirtualPlugin" type="somethingVirtualPlugin"/>
         </type>
 
         <type name="Magento\Theme\Model\View\Design">
@@ -23,4 +24,8 @@
                         </argument>
                 </arguments>
         </type>
+
+        <virtualType name="somethingVirtualPlugin" type="Ampersand\Test\Plugin\AdobeImsUserProfileVirtual">
+        </virtualType>
+
 </config>
diff --git a/dev/phpunit/functional/FunctionalTests.php b/dev/phpunit/functional/FunctionalTests.php
@@ -8,6 +8,10 @@ class FunctionalTests extends \PHPUnit\Framework\TestCase
      */
     private function generateAnalyseCommand($versionPath, $arguments = '')
     {
+        if (!str_contains($arguments, '--php-strict-errors')) {
+            $arguments.= ' --php-strict-errors ';
+        }
+
         $baseDir = BASE_DIR;
         $command = "php {$baseDir}/bin/patch-helper.php analyse $arguments {$baseDir}{$versionPath}";
         echo PHP_EOL . "Generated command: $command" . PHP_EOL;
@@ -39,6 +43,10 @@ public function testMagentoTwoTwo()
      */
     public function testVirtualTypesNoException()
     {
+        copy(
+            BASE_DIR . '/dev/instances/magento22/vendor.patch',
+            BASE_DIR . '/dev/instances/magento22/vendor.patch.bak'
+        );
         copy(
             BASE_DIR . '/dev/phpunit/functional/resources/reflection-exception.diff',
             BASE_DIR . '/dev/instances/magento22/vendor.patch'
@@ -50,6 +58,11 @@ public function testVirtualTypesNoException()
         );
 
         exec($this->generateAnalyseCommand('/dev/instances/magento22'), $output, $return);
+
+        copy(
+            BASE_DIR . '/dev/instances/magento22/vendor.patch.bak',
+            BASE_DIR . '/dev/instances/magento22/vendor.patch'
+        );
         $this->assertEquals(0, $return, "The return code of the command was not zero");
     }
 
@@ -96,6 +109,10 @@ public function testMagentoTwoThreeShowCustomModules()
      */
     public function testAutoApplyPatches()
     {
+        copy(
+            BASE_DIR . '/dev/instances/magento23/vendor.patch',
+            BASE_DIR . '/dev/instances/magento23/vendor.patch.bak'
+        );
         copy(
             BASE_DIR . '/dev/phpunit/functional/resources/template-change.diff',
             BASE_DIR . '/dev/instances/magento23/vendor.patch'
@@ -108,7 +125,10 @@ public function testAutoApplyPatches()
 
         exec($this->generateAnalyseCommand('/dev/instances/magento23', '--auto-theme-update 5'), $output, $return);
 
-        exec($this->generateAnalyseCommand('/dev/instances/magento23'), $output, $return);
+        copy(
+            BASE_DIR . '/dev/instances/magento23/vendor.patch.bak',
+            BASE_DIR . '/dev/instances/magento23/vendor.patch'
+        );
 
         $this->assertEquals(0, $return);
         $this->assertFileEquals(
@@ -125,6 +145,10 @@ public function testAutoApplyPatches()
      */
     public function testUnifiedDiffIsProvided()
     {
+        copy(
+            BASE_DIR . '/dev/instances/magento23/vendor.patch',
+            BASE_DIR . '/dev/instances/magento23/vendor.patch.bak'
+        );
         copy(
             BASE_DIR . '/dev/phpunit/functional/resources/not-a-unified-diff.txt',
             BASE_DIR . '/dev/instances/magento23/vendor.patch'
@@ -136,6 +160,10 @@ public function testUnifiedDiffIsProvided()
         );
 
         exec($this->generateAnalyseCommand('/dev/instances/magento23'), $output, $return);
+        copy(
+            BASE_DIR . '/dev/instances/magento23/vendor.patch.bak',
+            BASE_DIR . '/dev/instances/magento23/vendor.patch'
+        );
         $this->assertEquals(1, $return);
     }
 
@@ -156,4 +184,15 @@ public function testMagentoTwoFour()
 
         $this->assertEquals(\file_get_contents(BASE_DIR . '/dev/phpunit/functional/expected_output/magento24.out.txt'), $output);
     }
+
+    /**
+     * @group v24
+     */
+    public function testMagentoTwoFourVirtualPlugin()
+    {
+        $this->assertFileExists(BASE_DIR . '/dev/instances/magento24/app/etc/env.php', "Magento 2.4 is not installed");
+
+        exec($this->generateAnalyseCommand('/dev/instances/magento24'), $output, $return);
+        $this->assertEquals(0, $return, "The return code of the command was not zero");
+    }
 }
diff --git a/dev/phpunit/functional/expected_output/magento24.out.txt b/dev/phpunit/functional/expected_output/magento24.out.txt
@@ -10,6 +10,9 @@
 | Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                  | Ampersand\Test\Plugin\AdobeImsUserProfile::afterGetUpdatedAt                                     |
 | Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                  | Ampersand\Test\Plugin\AdobeImsUserProfile::aroundGetUpdatedAt                                    |
 | Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                  | Ampersand\Test\Plugin\AdobeImsUserProfile::beforeGetUpdatedAt                                    |
+| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                  | Ampersand\Test\Plugin\AdobeImsUserProfileVirtual::afterGetUpdatedAt                              |
+| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                  | Ampersand\Test\Plugin\AdobeImsUserProfileVirtual::aroundGetUpdatedAt                             |
+| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                  | Ampersand\Test\Plugin\AdobeImsUserProfileVirtual::beforeGetUpdatedAt                             |
 | Preference               | vendor/magento/module-weee/Model/Total/Quote/Weee.php                                  | Ampersand\Test\Model\Admin\Total\Quote\Weee                                                      |
 | Preference               | vendor/magento/module-weee/Model/Total/Quote/Weee.php                                  | Ampersand\Test\Model\Frontend\Total\Quote\Weee                                                   |
 | Preference               | vendor/magento/module-weee/Model/Total/Quote/Weee.php                                  | Ampersand\Test\Model\Total\Quote\Weee                                                            |

diff --git a/src/Ampersand/PatchHelper/Command/AnalyseCommand.php b/src/Ampersand/PatchHelper/Command/AnalyseCommand.php
@@ -26,11 +26,18 @@ protected function configure()
             )
             ->addOption('sort-by-type', null, InputOption::VALUE_NONE, 'Sort the output by override type')
             ->addOption('vendor-namespaces', null, InputOption::VALUE_OPTIONAL, 'Only show custom modules with these namespaces (comma separated list)')
+            ->addOption('php-strict-errors', null, InputOption::VALUE_NONE, 'Any php errors/warnings/notices will throw an exception')
             ->setDescription('Analyse a magento2 project which has had a ./vendor.patch file manually created');
     }
 
     protected function execute(InputInterface $input, OutputInterface $output)
     {
+        if ($input->getOption('php-strict-errors')) {
+            set_error_handler(function ($severity, $message, $file, $line) {
+                throw new \ErrorException($message, $severity, $severity, $file, $line);
+            });
+        }
+
         $projectDir = $input->getArgument('project');
         if (!(is_string($projectDir) && is_dir($projectDir))) {
             throw new \Exception("Invalid project directory specified");

diff --git a/src/Ampersand/PatchHelper/Helper/PatchOverrideValidator.php b/src/Ampersand/PatchHelper/Helper/PatchOverrideValidator.php
@@ -221,6 +221,22 @@ private function validatePhpFileForPlugins($vendorNamespaces = [])
                     }
                     $pluginClass = $pluginConf['instance'];
                     $pluginClass = ltrim($pluginClass, '\\');
+
+                    if (!class_exists($pluginClass) &&
+                        isset($areaConfig[$area][$pluginClass]['type']) &&
+                        class_exists($areaConfig[$area][$pluginClass]['type'])) {
+                        /*
+                         * The class doesn't exist but there is another reference to it in the area config
+                         * This is very likely a virtual type
+                         *
+                         * In our test case it is like this
+                         *
+                         * $pluginClass = somethingVirtualPlugin
+                         * $areaConfig['global']['somethingVirtualPlugin']['type'] = Ampersand\Test\Block\Plugin\OrderViewHistoryPlugin
+                         */
+                        $pluginClass = $areaConfig[$area][$pluginClass]['type'];
+                    }
+
                     if (!empty($vendorNamespaces)) {
                         foreach ($vendorNamespaces as $vendorNamespace) {
                             if (str_starts_with($pluginClass, $vendorNamespace)) {

diff --git a/src/Ampersand/PatchHelper/Patchfile/Reader.php b/src/Ampersand/PatchHelper/Patchfile/Reader.php
@@ -42,9 +42,12 @@ public function getFiles()
 
         while (!$this->file->eof()) {
             $line = $this->file->fgets();
-            if (str_starts_with($line, 'diff -ur ')) {
+            if (str_starts_with($line, 'diff ') && str_contains($line, '-ur')) {
                 $parts = explode(' ', $line);
-                $entry = new Entry($this->projectDir, $parts[3], $parts[2]);
+                // Work backwards from right to left, allows you to stack on additional diff params after diff -ur
+                $newFilePath = array_pop($parts);
+                $origFilePath = array_pop($parts);
+                $entry = new Entry($this->projectDir, $newFilePath, $origFilePath);
                 $files[] = $entry;
             }
             if (isset($entry)) {