Skip to content

Commit

Permalink
Add 'meaningful' checks to php (#114)
Browse files Browse the repository at this point in the history
Add 'meaningful' checks to php (#114)

Part of #110 

We do not look for "redundant" overrides as with class names etc it can be quite hard. We'll filter out comments/whitespace etc which should do a good job anyway. (This is inline with the issue as it was raised #85)

For ignoring non-meaningful changes these are split into two groups

- Preferences/Plugins - Downgrade from `WARN` to `IGNR` if the change is identified as non-meaningful.
- Setup/Schema/Etc - Do not even run the check, because they are `INFO` level anyway just ignore them and remove from the report entirely.

For example
```
| IGNR  | Preference               | vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/ToPreferenceAndIgnore.php                                       | Ampersand\Test\Model\ToPreferenceAndIgnore                                                                                         |
```

In this case `vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/ToPreferenceAndIgnore.php` had some whitespace/comment style change, and theres no need to report you have a preference of `Ampersand\Test\Model\ToPreferenceAndIgnore` defined.
  • Loading branch information
convenient authored Nov 7, 2023
1 parent a894a50 commit 470ba5e
Show file tree
Hide file tree
Showing 26 changed files with 255 additions and 22 deletions.
16 changes: 10 additions & 6 deletions dev/Docker/setup-magento-with-diff.sh
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,16 @@ echo "<!-- -->" >> vendor/ampersand/upgrade-patch-helper-test-module/src/module
echo "<!-- -->" >> vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/templates/cart/form.phtml # ensure that third party theme modifications show as expected
rm vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/ToPreferenceAndDelete.php
rm vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/ToPreferenceAndExtendAndDelete.php
echo "//some change" >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/SomeClass.php
echo "//some change" >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Api/ExampleInterface.php
echo "//some change" >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Api/ExampleTwoInterface.php
echo "//some change" >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/Patch/Schema/SomeSchemaChanges.php
echo "//some change" >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/Patch/Data/SomeDataChanges.php
echo "//some change" >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/InstallSchema.php
echo '//not-meaningful' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/ToPreferenceAndIgnore.php
echo '//not-meaningful' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/ToPluginAndIgnore.php
echo '$b=1;' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/SomeClass.php
echo '$b=1;' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Api/ExampleInterface.php
echo '$b=1;' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Api/ExampleTwoInterface.php
echo '$b=1;' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/Patch/Schema/SomeSchemaChanges.php
echo '$b=1;' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/Patch/Data/SomeDataChanges.php
echo '//not-meaningful' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/Patch/Schema/SomeSchemaNonChanges.php
echo '//not-meaningful' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/Patch/Data/SomeDataNonChanges.php
echo '$b=1;' >> vendor/ampersand/upgrade-patch-helper-test-module/src/module/Setup/InstallSchema.php
cp vendor/ampersand/upgrade-patch-helper-test-module/src/module/etc/db_schema.after.xml vendor/ampersand/upgrade-patch-helper-test-module/src/module/etc/db_schema.xml
rm vendor/ampersand/upgrade-patch-helper-test-module-to-be-removed/src/module/etc/db_schema.xml

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,15 @@

use Magento\Sales\Block\Adminhtml\Order\Create\Form;

/**
* This is useless phpdoc
*/
class OrderCreateFormPlugin
{
/**
* @param Form $subject
* @return void
*/
public function beforeGetOrderDataJson(Form $subject)
{
// do stuff
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?php
namespace Ampersand\Test\Model;

class ToPreferenceAndIgnore
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php
namespace Ampersand\Test\Plugin;

class ToPluginAndIgnore
{
public function beforeSuperCool($subject)
{
// do stuff
}
}
5 changes: 5 additions & 0 deletions dev/TestModule/app/code/Ampersand/Test/etc/di.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<preference for="Magento\Authorizenet\Model\Directpost" type="Ampersand\Test\Model\Directpost" />
<preference for="Magento\Weee\Model\Total\Quote\Weee" type="Ampersand\Test\Model\Total\Quote\Weee" />
<preference for="Ampersand\TestVendor\Model\SomeClass" type="Ampersand\Test\Model\ThirdPartyClass" />
<preference for="Ampersand\TestVendor\Model\ToPreferenceAndIgnore" type="Ampersand\Test\Model\ToPreferenceAndIgnore" />
<preference for="Ampersand\TestVendor\Model\ToPreferenceAndDelete" type="Ampersand\Test\Model\ToPreferenceAndDelete" />
<preference for="Ampersand\TestVendor\Model\ToPreferenceAndExtendAndDelete" type="Ampersand\Test\Model\ToPreferenceAndExtendAndDelete" />
<preference for="Magento\Framework\Locale\Format" type="Ampersand\Test\Model\Locale\Format" />
Expand All @@ -17,6 +18,10 @@
<plugin name="AmpersandTestPluginBeforeAfterAround2" type="Ampersand\Test\Block\Plugin\OrderViewHistoryPlugin" sortOrder="1" />
</type>

<type name="Ampersand\TestVendor\Model\ToPluginAndIgnore">
<plugin name="ToPluginAndIgnore" type="Ampersand\Test\Plugin\ToPluginAndIgnore" sortOrder="1" />
</type>

<type name="Magento\AdobeIms\Model\UserProfile">
<plugin name="AmpersandTestPluginBeforeAfterAround3\UserProfile" type="Ampersand\Test\Plugin\AdobeImsUserProfile" sortOrder="1" />
<plugin name="AmpersandTestPluginBeforeAfterAround3\UserProfileNoTypeSpecified" />
Expand Down
10 changes: 10 additions & 0 deletions dev/TestVendorModule/src/module/Model/ToPluginAndIgnore.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php
namespace Ampersand\TestVendor\Model;

class ToPluginAndIgnore
{
public function superCool()
{
return '1234';
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php
namespace Ampersand\TestVendor\Model;

class ToPreferenceAndIgnore
{
// Dummy class to be overridden
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php
namespace Ampersand\TestVendor\Setup\Patch\Data;

class SomeDataNonChanges implements \Magento\Framework\Setup\Patch\DataPatchInterface
{
public function getAliases()
{
return [];
}

public static function getDependencies()
{
return [];
}

public function apply()
{
return $this;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php
namespace Ampersand\TestVendor\Setup\Patch\Schema;

class SomeSchemaNonChanges implements \Magento\Framework\Setup\Patch\SchemaPatchInterface
{
public function getAliases()
{
return [];
}

public static function getDependencies()
{
return [];
}

public function apply()
{
return $this;
}
}
2 changes: 1 addition & 1 deletion dev/phpunit/functional/expected_output/magentom23.out.txt
Original file line number Diff line number Diff line change
Expand Up @@ -69,5 +69,5 @@
+-------+--------------------------+------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
WARN count: 65
INFO count: 309 (to view re-run this tool with --show-info)
IGNORE count: 4 (to view re-run this tool with --show-ignore)
IGNORE count: 5 (to view re-run this tool with --show-ignore)
For docs on each check see https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/CHECKS_AVAILABLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,6 @@
| WARN | Redundant Override | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Ui/web/templates/redundant.html | app/design/frontend/Ampersand/theme/Magento_Ui/web/templates/redundant.html |
+-------+--------------------------+------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
WARN count: 66
INFO count: 314 (to view re-run this tool with --show-info)
IGNORE count: 4 (to view re-run this tool with --show-ignore)
INFO count: 309 (to view re-run this tool with --show-info)
IGNORE count: 5 (to view re-run this tool with --show-ignore)
For docs on each check see https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/CHECKS_AVAILABLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,5 +64,5 @@
+-------+--------------------------+------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
WARN count: 60
INFO count: 395 (to view re-run this tool with --show-info)
IGNORE count: 5 (to view re-run this tool with --show-ignore)
IGNORE count: 6 (to view re-run this tool with --show-ignore)
For docs on each check see https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/CHECKS_AVAILABLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,9 @@
| IGNR | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Ui/web/templates/ignore.html | app/design/frontend/Ampersand/theme/Magento_Ui/web/templates/ignore.html |
| IGNR | Override (phtml/js/html) | vendor/magento/module-catalog/view/frontend/layout/catalog_category_view_type_default.xml | app/design/frontend/Ampersand/theme/Magento_Catalog/layout/catalog_category_view_type_default.xml |
| IGNR | Override (phtml/js/html) | vendor/magento/module-vault/view/frontend/web/js/view/payment/method-renderer/vault.js | app/design/frontend/Ampersand/theme/Magento_Vault/web/js/view/payment/method-renderer/vault.js |
| IGNR | Preference | vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/ToPreferenceAndIgnore.php | Ampersand\Test\Model\ToPreferenceAndIgnore |
+-------+--------------------------+------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
WARN count: 60
INFO count: 395
IGNORE count: 5
IGNORE count: 6
For docs on each check see https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/CHECKS_AVAILABLE.md
2 changes: 1 addition & 1 deletion dev/phpunit/functional/expected_output/magentom24.out.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,5 @@
+-------+--------------------------+------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
WARN count: 58
INFO count: 395 (to view re-run this tool with --show-info)
IGNORE count: 5 (to view re-run this tool with --show-ignore)
IGNORE count: 6 (to view re-run this tool with --show-ignore)
For docs on each check see https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/CHECKS_AVAILABLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,5 +64,5 @@
+-------+--------------------------+------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
WARN count: 60
INFO count: 384 (to view re-run this tool with --show-info)
IGNORE count: 5 (to view re-run this tool with --show-ignore)
IGNORE count: 6 (to view re-run this tool with --show-ignore)
For docs on each check see https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/CHECKS_AVAILABLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,5 +110,5 @@ phpstorm diff vendor/magento/theme-frontend-luma/etc/view.xml vendor/ampersand/u
phpstorm diff vendor/paypal/module-braintree-core/view/base/web/js/form-builder.js vendor/paypal/module-braintree-core/view/frontend/web/js/form-builder.js vendor_orig/paypal/module-braintree-core/view/base/web/js/form-builder.js
WARN count: 60
INFO count: 395 (to view re-run this tool with --show-info)
IGNORE count: 5 (to view re-run this tool with --show-ignore)
IGNORE count: 6 (to view re-run this tool with --show-ignore)
For docs on each check see https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/CHECKS_AVAILABLE.md
13 changes: 13 additions & 0 deletions dev/phpunit/unit/Ampersand/PatchHelper/Patchfile/SanitiserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,17 @@ public function stripWhitespaceAndMultilineDataProvider()
],
];
}

public function testStripCommentsFromPhp()
{
$contents = file_get_contents(BASE_DIR . '/dev/phpunit/unit/resources/patchfile/sanitiser/php/FooBarBaz.php');

$contents = Sanitiser::stripCommentsFromPhp($contents);
$contents = Sanitiser::stripWhitespaceAndNewlines($contents);

$this->assertEquals(
$contents,
file_get_contents(BASE_DIR . '/dev/phpunit/unit/resources/patchfile/sanitiser/php/FooBarBazExpected.php')
);
}
}
39 changes: 39 additions & 0 deletions dev/phpunit/unit/resources/patchfile/sanitiser/php/FooBarBaz.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php
// Some comment
namespace Foo\Bar;

/**
* yup
* yip
* yap
*/
class Baz {
//
//
//
/**
* @param $here
* @return void
*/
private function foobar() {
// some internal
/*
asdf
*/
$b=1;
/**
* comment
*/
}

/**
* @param there
* comment
*/
private function asdfasdf() {

}

} //
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php
namespace Foo\Bar;
class Baz {
/**
* @param $here
* @return void
*/
private function foobar() {
$b=1;
}
/**
* @param there
* comment
*/
private function asdfasdf() {
}
}
18 changes: 15 additions & 3 deletions src/Ampersand/PatchHelper/Checks/ClassPluginPhp.php
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,19 @@ class_exists($areaConfig[$area][$pluginClass]['type'])
}
if (isset($targetClassMethods) && is_array($targetClassMethods) && !empty($targetClassMethods)) {
if (isset($targetClassMethods[$methodName])) {
$this->warnings[Checks::TYPE_METHOD_PLUGIN_ENABLED][] = "$nonMagentoPlugin::$method";
if ($this->patchEntry->vendorChangeIsNotMeaningful()) {
$this->ignored[Checks::TYPE_METHOD_PLUGIN_ENABLED][] = "$nonMagentoPlugin::$method";
} else {
$this->warnings[Checks::TYPE_METHOD_PLUGIN_ENABLED][] = "$nonMagentoPlugin::$method";
}
}
} else {
// deleted handling
$this->warnings[Checks::TYPE_METHOD_PLUGIN_DISABLED][] = "$nonMagentoPlugin::$method";
if ($this->patchEntry->vendorChangeIsNotMeaningful()) {
$this->ignored[Checks::TYPE_METHOD_PLUGIN_DISABLED][] = "$nonMagentoPlugin::$method";
} else {
$this->warnings[Checks::TYPE_METHOD_PLUGIN_DISABLED][] = "$nonMagentoPlugin::$method";
}
}
}
}
Expand Down Expand Up @@ -209,7 +217,11 @@ class_exists($areaConfig[$area][$pluginClass]['type'])
if (!empty($intersection)) {
foreach ($intersection as $methods) {
foreach ($methods as $method) {
$this->warnings[Checks::TYPE_METHOD_PLUGIN][] = "$plugin::$method";
if ($this->patchEntry->vendorChangeIsNotMeaningful()) {
$this->ignored[Checks::TYPE_METHOD_PLUGIN][] = "$plugin::$method";
} else {
$this->warnings[Checks::TYPE_METHOD_PLUGIN][] = "$plugin::$method";
}
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/Ampersand/PatchHelper/Checks/ClassPreferencePhp.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ public function check()
}

foreach ($preferences as $preference) {
$this->warnings[$type][] = $preference;
if ($this->patchEntry->vendorChangeIsNotMeaningful()) {
$this->ignored[$type][] = $preference;
} else {
$this->warnings[$type][] = $preference;
}
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/Ampersand/PatchHelper/Checks/SetupPatchDataPhp.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ public function __construct(
public function canCheck()
{
$path = $this->patchEntry->getPath();
return str_contains($path, '/Setup/Patch/Data/') && pathinfo($path, PATHINFO_EXTENSION) === 'php';

return str_contains($path, '/Setup/Patch/Data/') &&
pathinfo($path, PATHINFO_EXTENSION) === 'php' &&
$this->patchEntry->vendorChangeIsMeaningful();
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/Ampersand/PatchHelper/Checks/SetupPatchSchemaPhp.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ public function __construct(
public function canCheck()
{
$path = $this->patchEntry->getPath();
return str_contains($path, '/Setup/Patch/Schema/') && pathinfo($path, PATHINFO_EXTENSION) === 'php';
return str_contains($path, '/Setup/Patch/Schema/') &&
pathinfo($path, PATHINFO_EXTENSION) === 'php' &&
$this->patchEntry->vendorChangeIsMeaningful();
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/Ampersand/PatchHelper/Checks/SetupScriptPhp.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public function canCheck()
return str_contains($path, '/Setup/')
&& !str_starts_with($path, '/vendor/magento/framework/')
&& !str_contains($path, '/Setup/Patch/')
&& pathinfo($path, PATHINFO_EXTENSION) === 'php';
&& pathinfo($path, PATHINFO_EXTENSION) === 'php'
&& $this->patchEntry->vendorChangeIsMeaningful();
}

/**
Expand Down
18 changes: 17 additions & 1 deletion src/Ampersand/PatchHelper/Patchfile/Entry.php
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,21 @@ public function vendorChangeIsNotMeaningful()
return $this->fileWasModified() && $this->sanitisedContentsAreTheSame();
}

/**
* Is the diff actually something that should investigated
*
* eg not
* - trailing/leading whitespace
* - a comment
* - extra newlines
*
* @return bool
*/
public function vendorChangeIsMeaningful()
{
return !$this->vendorChangeIsNotMeaningful();
}

/**
* Detect if the file exists both before and after, if so it was a modification
*
Expand Down Expand Up @@ -578,7 +593,8 @@ private function getSanitisedFileContents($file)
$contents = Sanitiser::stripCommentsFromXml($contents);
break;
case 'php':
break; // todo not implemented
$contents = Sanitiser::stripCommentsFromPhp($contents);
break;
case 'phtml':
break; // todo not implemented
case 'js':
Expand Down
Loading

0 comments on commit 470ba5e

Please sign in to comment.