Don't report files with non-meaningful code changes

[hostep]

Hi!

I recently was involved in a Magento upgrade from 2.1.12 to 2.1.16
In 2.1.13, Magento changed all copyright headers in all the files. This change is not important for comparing changes between versions of Magento. So files where only this copyright header got changed should not be outputted.

Some example files:

• vendor/magento/module-catalog/Model/ResourceModel/Eav/Attribute.php: 2.1.12 vs 2.1.16
• vendor/magento/module-catalog/view/base/templates/product/price/final_price.phtml: 2.1.12 vs 2.1.16

diff:

 <?php
 /**
- * Copyright © 2013-2017 Magento, Inc. All rights reserved.
+ * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */

Another one which can be observed between Magento 2.1.x and 2.2.x are those php short tags in phtml files. Where it has changed from <?php echo ... to <?= ....

For example in: vendor/magento/module-catalog/view/frontend/templates/product/list/toolbar.phtml 2.1.16 vs 2.2.7

diff:

 use Magento\Catalog\Model\Product\ProductList\Toolbar;
 ?>
 <?php if ($block->getCollection()->getSize()): ?>
-    <div class="toolbar toolbar-products" data-mage-init='<?php /* @escapeNotVerified */ echo $block->getWidgetOptionsJson(); ?>'>
+    <div class="toolbar toolbar-products" data-mage-init='<?= /* @escapeNotVerified */ $block->getWidgetOptionsJson() ?>'>
         <?php if ($block->isExpanded()): ?>
             <?php include ($block->getTemplateFile('Magento_Catalog::product/list/toolbar/viewmode.phtml')) ?>
         <?php endif; ?>
 
         <?php include ($block->getTemplateFile('Magento_Catalog::product/list/toolbar/amount.phtml')) ?>
 
-        <?php echo $block->getPagerHtml() ?>
+        <?= $block->getPagerHtml() ?>
 
         <?php include ($block->getTemplateFile('Magento_Catalog::product/list/toolbar/limiter.phtml')) ?>

Also notice the extra ; which was removed in the first diff line (is also non-meaningful in this case)
But those escapeNotVerified comments (which exist in a couple of different forms) might make things more complex.

Would it somehow be possible to ignore these non-meaningful code changes?

Some patterns which I think can be ignored:

• whitespace-only changes (can probably be fixed with adding the -w flag to the diff command?)
• copyright header changes
• everything which is in comments and has changed (not sure about php docblocks?)
• <?php echo ... vs <?= ... in phtml files
• extra ; at the end of a single php line in phtml files
• ...

Not sure how easy it is for something like this to be implemented?

[convenient]

Hey @hostep

This is a very interesting idea. Skirting around the edges of making this tool have sort of semantic diff functionality?

The copyright header was a massive pain wasn't it. All of our projects have bypassed that version so I'm thankful we won't have to deal with it again.

As far as actually implementing something like this...

In the Patchfile\Entry class we could create a new method like $entry->isMeaningful(). We may have to have different specifications for what is meaningful depending on the file extension. (If no rules are defined default to true rather than false?)

We already have the individual lines removed/added. We could refactor this around a bit and add rules for getting the modified lines and getting the "meaningful" modified lines.

ampersand-magento2-upgrade-patch-helper/src/Ampersand/PatchHelper/Patchfile/Entry.php

Lines 153 to 164 in ddcaf26

	$hunks = $this->getHunks();
	$modifiedLines = $this->getModifiedLines($hunks);
	$affectedFunctions = [];
	foreach ($modifiedLines['new'] as $lineNumber => $expectedLine) {
	$affectedFunctions[] = $this->getAffectedFunction($lineNumber, $newFileContents, $expectedLine);
	}
	foreach ($modifiedLines['original'] as $lineNumber => $expectedLine) {
	$affectedFunctions[] = $this->getAffectedFunction($lineNumber, $originalFileContents, $expectedLine);
	}

We could then use this function in PatchOverrideValidator and bail out early if the diff in question is not meaningful. Something like this perhaps.

    public function validate()
    {
+       if (!$this->patchEntry->isMeaningful()) { return true; }
        switch (pathinfo($this->vendorFilepath, PATHINFO_EXTENSION)) {
            case 'php':
                $this->validatePhpFileForPreferences();
                $this->validatePhpFileForPlugins();

I think the big issue is "what is meaningful?" I know docblock params are used by the swagger api so selectively ignoring them may be tricky.

I'm not sure how i'd feel if this tool became too opinionated about the diffs in this manner, but i'd be all for having it as a --only-meaningful-diffs.

We should probably just double check that there's not a better term than meaningful before codifying this as well. I feel like this is similar to a semantic diff but that doesnt feel 100% either.

Let me know if you have any further thoughts. I won't be getting a chance to look at this feature in the foreseeable future so will add an appropriate label to this issue.

Thanks for the feedback 👌