Report redundant overrides, and ignore non-meaningful changes in vendor
#110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To capture cases where a warning is being demoted, because the actual vendor file changed in some non meaningful way.
This was
linked to
issues
Oct 31, 2023
Implement "meaningful" checks for `.html` email templates. In this case `vendor/ampersand/upgrade-patch-helper-test-module/src/module/view/frontend/email/ignore.html` had some changes that were a comment or whitespace, so not worth actually reporting. It's flagged as ignored and hidden by default. ``` | IGNR | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-module/src/module/view/frontend/email/ignore.html | app/design/frontend/Ampersand/theme/Ampersand_TestVendor/email/ignore.html | ``` In this case `vendor/ampersand/upgrade-patch-helper-test-module/src/module/view/frontend/email/redundant.html` had some changes made to it, and the contents now equate to `app/design/frontend/Ampersand/theme/Ampersand_TestVendor/email/redundant.html` so the local `app/design` copy is redundant and can be removed. ``` | WARN | Redundant Override | vendor/ampersand/upgrade-patch-helper-test-module/src/module/view/frontend/email/redundant.html | app/design/frontend/Ampersand/theme/Ampersand_TestVendor/email/redundant.html | ```
Implement 'meaningful' checks to `.js` Part of #110 ## Ignore Override Warning ``` | IGNR | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/web/js/ignore.js | app/design/frontend/Ampersand/theme/Magento_Checkout/web/js/ignore.js | ``` In this case `vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/web/js/ignore.js` was updated with some whitespace/comment/etc so its not really changed, so don't bother reporting on it. The `IGNR` entries are suppressed without `--show-ignore` ## Redundant override ``` | WARN | Redundant Override | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/web/js/redundant.js | app/design/frontend/Ampersand/theme/Magento_Checkout/web/js/redundant.js | ``` In this case `vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/web/js/redundant.js` has been updated and is now equivalent (excluding some whitespace/comments) to `app/design/frontend/Ampersand/theme/Magento_Checkout/web/js/redundant.js` The `app/design` override is now redundant and not necessary ## Additional test case changes The additional test cases needed updated as this file is being ignored, due to the fact its only a comment / whitespace from magento/magento2@1697149 Previously a human would have had to look at the change and work it out themselves, now its not reported. ```diff diff -ur -N vendor_orig/magento/module-vault/view/frontend/web/js/view/payment/vault.js vendor/magento/module-vault/view/frontend/web/js/view/payment/vault.js --- vendor_orig/magento/module-vault/view/frontend/web/js/view/payment/vault.js 2020-04-13 17:35:44.000000000 +0000 +++ vendor/magento/module-vault/view/frontend/web/js/view/payment/vault.js 2023-06-07 19:25:10.000000000 +0000 @@ -3,7 +3,7 @@ * See COPYING.txt for license details. */ /*browser:true*/ -/*global define*/ + /* @api */ define([ 'underscore', ``` And these files are being flagged as redundant, because `vendor/paypal/module-braintree-core/view/base/web/js/form-builder.js` has been edited/changed and is now the same as `vendor/paypal/module-braintree-core/view/frontend/web/js/form-builder.js` ``` e676f80e153b2a8e55da763f237cabf4 vendor/paypal/module-braintree-core/view/base/web/js/form-builder.js e676f80e153b2a8e55da763f237cabf4 vendor/paypal/module-braintree-core/view/frontend/web/js/form-builder.js ```
Add 'meaningful' checks to XML files ## Ignore Override Warning ``` | 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 | ``` In this case `vendor/magento/module-catalog/view/frontend/layout/catalog_category_view_type_default.xml` was updated with some whitespace/comment/etc so its not really changed, so don't bother reporting on it. The `IGNR` entries are suppressed without `--show-ignore` ## Redundant override ``` | WARN | Redundant Override | vendor/magento/theme-frontend-blank/etc/view.xml | vendor/ampersand/upgrade-patch-helper-test-hyva-fallback-theme/theme/etc/view.xml | ``` In this case `vendor/magento/theme-frontend-blank/etc/view.xml` has been updated and is now equivalent (excluding some whitespace/comments) to `vendor/ampersand/upgrade-patch-helper-test-hyva-fallback-theme/theme/etc/view.xml`
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.
* initial attempt to sanitise phtml * Update Sanitiser.php * Update Entry.php * Update test cases * Update test cases * Update test cases * Update test cases * Update test cases * Update test cases
https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper into poc-meaningful-semantic-understanding-of-file-changes
|
@hostep @sprankhub I think this is it, there's probably room for improvement as we get some real world experience of how these checks come through but would be curious to see if you have any rough thoughts. @peterjaap FYI new |
|
Great job! Awesome work!! Just gave it a try on a extremely customised project that we're currently upgrading from 2.3.6 to 2.4.6.
Examplesdiff -ur -N vendor_orig/magento/module-bundle/view/frontend/templates/catalog/product/view/summary.phtml vendor/magento/module-bundle/view/frontend/templates/catalog/product/view/summary.phtml
--- vendor_orig/magento/module-bundle/view/frontend/templates/catalog/product/view/summary.phtml 2023-11-08 18:34:17
+++ vendor/magento/module-bundle/view/frontend/templates/catalog/product/view/summary.phtml 2023-07-21 09:34:42
@@ -3,10 +3,9 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
-?>
-<?php
- $_product = $block->getProduct();
+// phpcs:disable PHPCompatibility.Miscellaneous.RemovedAlternativePHPTags.MaybeASPOpenTagFound
+$_product = $block->getProduct();
?>
<?php if ($_product->isSaleable() && $block->hasOptions()) : ?>
<div id="bundleSummary"
diff -ur -N vendor_orig/magento/module-wishlist/view/frontend/templates/sidebar.phtml vendor/magento/module-wishlist/view/frontend/templates/sidebar.phtml
--- vendor_orig/magento/module-wishlist/view/frontend/templates/sidebar.phtml 2023-11-08 18:34:13
+++ vendor/magento/module-wishlist/view/frontend/templates/sidebar.phtml 2023-02-23 14:11:02
@@ -7,7 +7,6 @@
/** @var \Magento\Wishlist\Block\Customer\Sidebar $block */
?>
<?php
-/** @var \Magento\Wishlist\ViewModel\WishlistData $wishlstViewModel */
$wishlstViewModel = $block->getData('wishlistDataViewModel');
?>
<?php if ($wishlstViewModel->isAllow()): ?>
diff -ur -N vendor_orig/magento/module-wishlist/view/frontend/templates/view.phtml vendor/magento/module-wishlist/view/frontend/templates/view.phtml
--- vendor_orig/magento/module-wishlist/view/frontend/templates/view.phtml 2023-11-08 18:34:13
+++ vendor/magento/module-wishlist/view/frontend/templates/view.phtml 2023-02-23 14:11:02
@@ -4,6 +4,7 @@
* See COPYING.txt for license details.
*/
+// phpcs:disable PHPCompatibility.Miscellaneous.RemovedAlternativePHPTags.MaybeASPOpenTagFound
/** @var \Magento\Wishlist\Block\Customer\Wishlist $block */
?>
diff -ur -N vendor_orig/magento/framework/Pricing/PriceCurrencyInterface.php vendor/magento/framework/Pricing/PriceCurrencyInterface.php
--- vendor_orig/magento/framework/Pricing/PriceCurrencyInterface.php 2023-11-08 18:34:12
+++ vendor/magento/framework/Pricing/PriceCurrencyInterface.php 2023-09-22 08:57:04
@@ -14,9 +14,6 @@
*/
interface PriceCurrencyInterface
{
- /**
- * Default precision
- */
const DEFAULT_PRECISION = 2;
/**
@@ -48,7 +45,7 @@
* @param int $precision
* @param null|string|bool|int|\Magento\Framework\App\ScopeInterface $scope
* @param \Magento\Framework\Model\AbstractModel|string|null $currency
- * @return float
+ * @return string
*/
public function format(
$amount,
diff -ur -N vendor_orig/magento/module-catalog-inventory/Block/Stockqty/DefaultStockqty.php vendor/magento/module-catalog-inventory/Block/Stockqty/DefaultStockqty.php
--- vendor_orig/magento/module-catalog-inventory/Block/Stockqty/DefaultStockqty.php 2023-11-08 18:34:14
+++ vendor/magento/module-catalog-inventory/Block/Stockqty/DefaultStockqty.php 2023-07-21 09:34:44
@@ -13,8 +13,8 @@
* @since 100.0.2
*
* @deprecated 100.3.0 Replaced with Multi Source Inventory
- * @link https://devdocs.magento.com/guides/v2.3/inventory/index.html
- * @link https://devdocs.magento.com/guides/v2.3/inventory/catalog-inventory-replacements.html
+ * @link https://devdocs.magento.com/guides/v2.4/inventory/index.html
+ * @link https://devdocs.magento.com/guides/v2.4/inventory/inventory-api-reference.html
*/
class DefaultStockqty extends AbstractStockqty implements \Magento\Framework\DataObject\IdentityInterface
{
diff -ur -N vendor_orig/magento/module-catalog/Api/Data/ProductAttributeInterface.php vendor/magento/module-catalog/Api/Data/ProductAttributeInterface.php
--- vendor_orig/magento/module-catalog/Api/Data/ProductAttributeInterface.php 2023-11-08 18:34:19
+++ vendor/magento/module-catalog/Api/Data/ProductAttributeInterface.php 2023-09-22 08:56:56
@@ -34,8 +34,6 @@
const CODE_WEIGHT = 'weight';
/**
- * Get extension attributes
- *
* @return \Magento\Eav\Api\Data\AttributeExtensionInterface|null
* @since 103.0.0
*/
diff -ur -N vendor_orig/magento/module-catalog/Block/Product/ReviewRendererInterface.php vendor/magento/module-catalog/Block/Product/ReviewRendererInterface.php
--- vendor_orig/magento/module-catalog/Block/Product/ReviewRendererInterface.php 2023-11-08 18:34:18
+++ vendor/magento/module-catalog/Block/Product/ReviewRendererInterface.php 2023-09-22 08:56:56
@@ -9,6 +9,7 @@
/**
* Interface \Magento\Catalog\Block\Product\ReviewRendererInterface
*
+ * @api
*/
interface ReviewRendererInterface
{Just one thing I'm missing is the mention of this new Again, awesome job! 👏 |
convenient
deleted the
poc-meaningful-semantic-understanding-of-file-changes
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to solve the following, both of these combined should allow you to do less work, and the work you do have to do should be more targeted
Both of those issues require some understanding of what is "meaningful", so the start of that is stripping of whitespace/comments/multi newlines.
With the ability to understand what the actual change made in vendor looks like, we can hopefully ignore a lot of redundant checks. And in the event that the overridden file is now redundant itself, report that.
In this PR I have scaffolded the rough setup of how I think this can work, as well as some test cases for
.htmltemplate overrides. I will link subsequent PRs pointing to this branch as I implement each check type just to try and keep it easier to review and grok what i'm doing. For example of each type and what can be ignored or flagged as redundant click through.js#112I have added a new type called
IGNRwhich is visible with--show-ignore, this is because I don't want to risk binning off anything that was previously highlighted and this way we can still investigate just in case there is a bug or something.I see three types of cases
Case 1 - Current base functionality
WARN, needs review as standardCase 2 - A NOOP change (comment/whitespace/etc)
IGNRas it's something to be filtered (hidden until ran with--show-ignore)Case 3 - Changed to match the override, maybe the override was backported but no longer necessary
WARN, its a redundant overrideUpdated documentation can be seen
There's a lot of moving pieces and a bunch of test code required here which makes this PR quite chunky at 93, but looking at results may be the best way to get a sense of what is achieved here.
Checklist