Possibility to open three-file-diff viewer directly from the output

norgeindian commented 2 years ago

Firstly: Thanks a lot for developing this awesome helper. We use it for every Magento update, and it makes our live so much easier! When I now have the output of which files I shall check, I get through them one by one and open the diff viewer between the override / preference and the Magento Core file. Sometimes, if I'm still not sure, if the changes are needed, I open the Core file in the vendor_orig folder and check out the three-file-diff. Do you see any good way of having that directly as a possibility? Maybe with a clickable link, which opens that view? I saw, that PHPStorm offers some CLI approach: https://www.jetbrains.com/help/phpstorm/command-line-differences-viewer.html For this, we would need the paths of all three files, which might be a bit difficult. What do you think?

convenient commented 2 years ago

Hey @norgeindian

Thanks for the issue.

Currently this tooling is pretty light on what you do with the output vendor_files_to_check.patch and the output from the CLI command. Having something clickable and actually interactable isn't something I'm planning on working on any time soon.

However, it does sound more like the kind of thing provided by @peterjaap and his tool https://github.com/elgentos/magento2-upgrade-gui#screenshots , have you given it a look and seen if it does what you need?

For this, we would need the paths of all three files, which might be a bit difficult.

So from the patchfile itself we have old/new path, but not override path when it is a PHP class.

Perhaps some changes could be made here (and similar for the plugin) to maybe use reflection and the object manager to instantiate the class, then get the filepath. and put that in the output instead? This could be gated with some --to-check-show-filepaths or something perhaps?

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/7c0e61803a85c8f38f0215684578f86956f0a9b4/src/Ampersand/PatchHelper/Helper/PatchOverrideValidator.php#L192

peterjaap commented 2 years ago

@convenient I solve that by dumping the composer classmap with composer dump --classmap-authoritative, this will give you a mapping of the classname to the file;

composer dump --classmap-authoritative
php -r "\$classmap=require_once('vendor/composer/autoload_classmap.php'); echo json_encode(\$classmap);" > classmap.json

convenient commented 2 years ago

@norgeindian @peterjaap It seems getting actual filepaths are pretty reasonable

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/compare/convert-class-names-to-filepaths?expand=1

I didn't bother putting it behind a flag as this was a quick test, but seems simple enough.

Is this the kind of thing you need? Would it make sense to dump it into a different/new column on the table if the flag is present?

| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/email/password_reset_confirmation.html   | app/design/frontend/Ampersand/theme/Magento_Customer/email/password_reset_confirmation.html |
| Override (phtml/js/html) | vendor/magento/module-sales/view/frontend/layout/sales_order_print.xml                | app/design/frontend/Ampersand/theme/Magento_Sales/layout/sales_order_print.xml              |
| Override (phtml/js/html) | vendor/magento/module-ui/view/base/web/templates/grid/masonry.html                    | app/design/frontend/Ampersand/theme/Magento_Ui/web/templates/grid/masonry.html              |
| Plugin                   | vendor/magento/framework/Stdlib/Cookie/PhpCookieManager.php                           | app/code/Ampersand/Test/Plugin/PhpCookieManager.php (beforeSetPublicCookie)                 |
| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                 | app/code/Ampersand/Test/Plugin/AdobeImsUserProfile.php (afterGetUpdatedAt)                  |
| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                 | app/code/Ampersand/Test/Plugin/AdobeImsUserProfile.php (aroundGetUpdatedAt)                 |
| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                 | app/code/Ampersand/Test/Plugin/AdobeImsUserProfile.php (beforeGetUpdatedAt)                 |
| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                 | app/code/Ampersand/Test/Plugin/AdobeImsUserProfileVirtual.php (afterGetUpdatedAt)           |
| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                 | app/code/Ampersand/Test/Plugin/AdobeImsUserProfileVirtual.php (aroundGetUpdatedAt)          |
| Plugin                   | vendor/magento/module-adobe-ims/Model/UserProfile.php                                 | app/code/Ampersand/Test/Plugin/AdobeImsUserProfileVirtual.php (beforeGetUpdatedAt)          |
| Preference               | vendor/ampersand/upgrade-patch-helper-test-module/src/module/Api/ExampleInterface.php | app/code/Ampersand/Test/Model/Example.php                                                   |
| Preference               | vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/SomeClass.php      | app/code/Ampersand/Test/Model/ThirdPartyClass.php                                           |
| Preference               | vendor/magento/framework/Locale/Format.php                                            | app/code/Ampersand/Test/Model/Locale/Format.php                                             |
| Preference               | vendor/magento/module-advanced-pricing-import-export/Model/Export/AdvancedPricing.php | app/code/Ampersand/Test/Model/Admin/Export/AdvancedPricing.php                              |
| Preference               | vendor/magento/module-weee/Model/Total/Quote/Weee.php                                 | app/code/Ampersand/Test/Model/Admin/Total/Quote/Weee.php                                    |
| Preference               | vendor/magento/module-weee/Model/Total/Quote/Weee.php                                 | app/code/Ampersand/Test/Model/Frontend/Total/Quote/Weee.php                                 |
| Preference               | vendor/magento/module-weee/Model/Total/Quote/Weee.php                                 | app/code/Ampersand/Test/Model/Total/Quote/Weee.php                                          |
| Queue consumer added     | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml                        | inventory.indexer.sourceItem                                                                |
| Queue consumer added     | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml                        | inventory.indexer.stock                                                                     |
| Queue consumer added     | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml                        | inventory.reservations.updateSalabilityStatus                                               |

norgeindian commented 2 years ago

@convenient , awesome, that would already help a lot and I think with a flag it could be added to the same column. As far as I see, there is no need to have both in the table. I'm already in contact with @peterjaap , discussing if this might fit better into his tool. Do you see any way to output the whole command in one, so it can be easily copied directly? That would then look like:

phpstorm.sh diff vendor/magento/module-customer/view/frontend/email/password_reset_confirmation.html app/design/frontend/Ampersand/theme/Magento_Customer/email/password_reset_confirmation.html  vendor_orig/magento/module-customer/view/frontend/email/password_reset_confirmation.html

I know, pretty lengthy, but would help me a lot and might it make easier for others as well.

convenient commented 2 years ago

@norgeindian So i've had a hack around in https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/compare/convert-class-names-to-filepaths?expand=1

It makes this function/class a bit messy and could probably do with a bit of refactoring to prevent it being too bloated

But with that you could probably pass output-diff-commands='phpstorm.sh' to this tool and get output like

+--------------------------+-----------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------+
| Type                     | Core                                                                                                            | To Check                                                                                                               |
+--------------------------+-----------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------+
| Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-module/src/module/view/frontend/templates/checkout/something.phtml   | app/design/frontend/Ampersand/theme/Ampersand_TestVendor/templates/checkout/something.phtml                            |
| Override (phtml/js/html) | vendor/magento/module-bundle/view/frontend/templates/js/components.phtml                                        | app/design/frontend/Ampersand/theme/Magento_Bundle/templates/js/components.phtml                                       |
| Override (phtml/js/html) | vendor/magento/module-catalog-widget/view/frontend/templates/product/widget/content/grid.phtml                  | app/design/frontend/Ampersand/theme/Magento_CatalogWidget/templates/product/widget/content/grid.phtml                  |
| Override (phtml/js/html) | vendor/magento/module-catalog/view/frontend/layout/catalog_category_view.xml                                    | app/design/frontend/Ampersand/theme/Magento_Catalog/layout/catalog_category_view.xml                                   |
| Override (phtml/js/html) | vendor/magento/module-checkout/view/frontend/templates/cart/form.phtml                                          | app/design/frontend/Ampersand/theme/Magento_Checkout/templates/cart/form.phtml                                         |
| Override (phtml/js/html) | vendor/magento/module-checkout/view/frontend/templates/cart/form.phtml                                          | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/templates/cart/form.phtml                 |
| Override (phtml/js/html) | vendor/magento/module-checkout/view/frontend/web/js/model/place-order.js                                        | app/design/frontend/Ampersand/theme/Magento_Checkout/web/js/model/place-order.js                                       |
| Override (phtml/js/html) | vendor/magento/module-checkout/view/frontend/web/template/summary/item/details.html                             | app/design/frontend/Ampersand/theme/Magento_Checkout/web/template/summary/item/details.html                            |
| Override (phtml/js/html) | vendor/magento/module-configurable-product/view/frontend/templates/product/view/type/options/configurable.phtml | app/design/frontend/Ampersand/theme/Magento_ConfigurableProduct/templates/product/view/type/options/configurable.phtml |
| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/email/password_reset_confirmation.html                             | app/design/frontend/Ampersand/theme/Magento_Customer/email/password_reset_confirmation.html                            |
| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/templates/account/dashboard/info.phtml                             | app/design/frontend/Ampersand/theme/Magento_Customer/templates/account/dashboard/info.phtml                            |
| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/web/js/model/authentication-popup.js                               | app/design/frontend/Ampersand/theme/Magento_Customer/web/js/model/authentication-popup.js                              |
| Override (phtml/js/html) | vendor/magento/module-sales/view/frontend/layout/sales_order_print.xml                                          | app/design/frontend/Ampersand/theme/Magento_Sales/layout/sales_order_print.xml                                         |
| Override (phtml/js/html) | vendor/magento/module-ui/view/base/web/templates/block-loader.html                                              | app/design/frontend/Ampersand/theme/Magento_Ui/web/templates/block-loader.html                                         |
| Plugin                   | vendor/magento/module-sales/Block/Adminhtml/Order/Create/Form.php                                               | Ampersand\Test\Block\Plugin\OrderCreateFormPlugin::afterGetOrderDataJson                                               |
| Plugin                   | vendor/magento/module-sales/Block/Adminhtml/Order/Create/Form.php                                               | Ampersand\Test\Block\Plugin\OrderCreateFormPlugin::aroundGetOrderDataJson                                              |
| Plugin                   | vendor/magento/module-sales/Block/Adminhtml/Order/Create/Form.php                                               | Ampersand\Test\Block\Plugin\OrderCreateFormPlugin::beforeGetOrderDataJson                                              |
| Plugin                   | vendor/magento/module-sales/Block/Adminhtml/Order/View/History.php                                              | Ampersand\Test\Block\Plugin\OrderViewHistoryPlugin::afterCanSendCommentEmail                                           |
| Plugin                   | vendor/magento/module-sales/Block/Adminhtml/Order/View/History.php                                              | Ampersand\Test\Block\Plugin\OrderViewHistoryPlugin::aroundCanSendCommentEmail                                          |
| Plugin                   | vendor/magento/module-sales/Block/Adminhtml/Order/View/History.php                                              | Ampersand\Test\Block\Plugin\OrderViewHistoryPlugin::beforeCanSendCommentEmail                                          |
| Preference               | vendor/ampersand/upgrade-patch-helper-test-module/src/module/Api/ExampleInterface.php                           | Ampersand\Test\Model\Example                                                                                           |
| Preference               | vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/SomeClass.php                                | Ampersand\Test\Model\ThirdPartyClass                                                                                   |
| Preference               | vendor/magento/framework/Locale/Format.php                                                                      | Ampersand\Test\Model\Locale\Format                                                                                     |
| Preference               | vendor/magento/module-advanced-pricing-import-export/Model/Export/AdvancedPricing.php                           | Ampersand\Test\Model\Admin\Export\AdvancedPricing                                                                      |
| Preference               | vendor/magento/module-authorizenet/Model/Directpost.php                                                         | Ampersand\Test\Model\Admin\Directpost                                                                                  |
| Preference               | vendor/magento/module-authorizenet/Model/Directpost.php                                                         | Ampersand\Test\Model\Directpost                                                                                        |
| Preference               | vendor/magento/module-authorizenet/Model/Directpost.php                                                         | Ampersand\Test\Model\Frontend\Directpost                                                                               |
| 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                                                                                  |
+--------------------------+-----------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------+
Outputting diff commands below
phpstorm.sh diff vendor/ampersand/upgrade-patch-helper-test-module/src/module/view/frontend/templates/checkout/something.phtml app/design/frontend/Ampersand/theme/Ampersand_TestVendor/templates/checkout/something.phtml
phpstorm.sh diff vendor/magento/module-bundle/view/frontend/templates/js/components.phtml app/design/frontend/Ampersand/theme/Magento_Bundle/templates/js/components.phtml
phpstorm.sh diff vendor/magento/module-catalog-widget/view/frontend/templates/product/widget/content/grid.phtml app/design/frontend/Ampersand/theme/Magento_CatalogWidget/templates/product/widget/content/grid.phtml
phpstorm.sh diff vendor/magento/module-catalog/view/frontend/layout/catalog_category_view.xml app/design/frontend/Ampersand/theme/Magento_Catalog/layout/catalog_category_view.xml
phpstorm.sh diff vendor/magento/module-checkout/view/frontend/templates/cart/form.phtml app/design/frontend/Ampersand/theme/Magento_Checkout/templates/cart/form.phtml
phpstorm.sh diff vendor/magento/module-checkout/view/frontend/templates/cart/form.phtml vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/templates/cart/form.phtml
phpstorm.sh diff vendor/magento/module-checkout/view/frontend/web/js/model/place-order.js app/design/frontend/Ampersand/theme/Magento_Checkout/web/js/model/place-order.js
phpstorm.sh diff vendor/magento/module-checkout/view/frontend/web/template/summary/item/details.html app/design/frontend/Ampersand/theme/Magento_Checkout/web/template/summary/item/details.html
phpstorm.sh diff vendor/magento/module-configurable-product/view/frontend/templates/product/view/type/options/configurable.phtml app/design/frontend/Ampersand/theme/Magento_ConfigurableProduct/templates/product/view/type/options/configurable.phtml
phpstorm.sh diff vendor/magento/module-customer/view/frontend/email/password_reset_confirmation.html app/design/frontend/Ampersand/theme/Magento_Customer/email/password_reset_confirmation.html
phpstorm.sh diff vendor/magento/module-customer/view/frontend/templates/account/dashboard/info.phtml app/design/frontend/Ampersand/theme/Magento_Customer/templates/account/dashboard/info.phtml
phpstorm.sh diff vendor/magento/module-customer/view/frontend/web/js/model/authentication-popup.js app/design/frontend/Ampersand/theme/Magento_Customer/web/js/model/authentication-popup.js
phpstorm.sh diff vendor/magento/module-sales/view/frontend/layout/sales_order_print.xml app/design/frontend/Ampersand/theme/Magento_Sales/layout/sales_order_print.xml
phpstorm.sh diff vendor/magento/module-ui/view/base/web/templates/block-loader.html app/design/frontend/Ampersand/theme/Magento_Ui/web/templates/block-loader.html
phpstorm.sh diff vendor/magento/module-sales/Block/Adminhtml/Order/Create/Form.php app/code/Ampersand/Test/Block/Plugin/OrderCreateFormPlugin.php
phpstorm.sh diff vendor/magento/module-sales/Block/Adminhtml/Order/Create/Form.php app/code/Ampersand/Test/Block/Plugin/OrderCreateFormPlugin.php
phpstorm.sh diff vendor/magento/module-sales/Block/Adminhtml/Order/Create/Form.php app/code/Ampersand/Test/Block/Plugin/OrderCreateFormPlugin.php
phpstorm.sh diff vendor/magento/module-sales/Block/Adminhtml/Order/View/History.php app/code/Ampersand/Test/Block/Plugin/OrderViewHistoryPlugin.php
phpstorm.sh diff vendor/magento/module-sales/Block/Adminhtml/Order/View/History.php app/code/Ampersand/Test/Block/Plugin/OrderViewHistoryPlugin.php
phpstorm.sh diff vendor/magento/module-sales/Block/Adminhtml/Order/View/History.php app/code/Ampersand/Test/Block/Plugin/OrderViewHistoryPlugin.php
phpstorm.sh diff vendor/ampersand/upgrade-patch-helper-test-module/src/module/Api/ExampleInterface.php app/code/Ampersand/Test/Model/Example.php
phpstorm.sh diff vendor/ampersand/upgrade-patch-helper-test-module/src/module/Model/SomeClass.php app/code/Ampersand/Test/Model/ThirdPartyClass.php
phpstorm.sh diff vendor/magento/framework/Locale/Format.php app/code/Ampersand/Test/Model/Locale/Format.php
phpstorm.sh diff vendor/magento/module-advanced-pricing-import-export/Model/Export/AdvancedPricing.php app/code/Ampersand/Test/Model/Admin/Export/AdvancedPricing.php
phpstorm.sh diff vendor/magento/module-authorizenet/Model/Directpost.php app/code/Ampersand/Test/Model/Admin/Directpost.php
phpstorm.sh diff vendor/magento/module-authorizenet/Model/Directpost.php app/code/Ampersand/Test/Model/Directpost.php
phpstorm.sh diff vendor/magento/module-authorizenet/Model/Directpost.php app/code/Ampersand/Test/Model/Frontend/Directpost.php
phpstorm.sh diff vendor/magento/module-weee/Model/Total/Quote/Weee.php app/code/Ampersand/Test/Model/Admin/Total/Quote/Weee.php
phpstorm.sh diff vendor/magento/module-weee/Model/Total/Quote/Weee.php app/code/Ampersand/Test/Model/Frontend/Total/Quote/Weee.php
phpstorm.sh diff vendor/magento/module-weee/Model/Total/Quote/Weee.php app/code/Ampersand/Test/Model/Total/Quote/Weee.php'

peterjaap commented 2 years ago

@convenient but that only has a two-way diff (two filenames in the command)?

peterjaap commented 2 years ago

@convenient also see https://github.com/elgentos/magento2-upgrade-gui/issues/4#issuecomment-1268258135

convenient commented 2 years ago

Right so actually remembering the requirements my quick hack attempt which needs some refactoring and considerations for code styling would look like so @peterjaap @norgeindian

In this rough PR https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/pull/71

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/e7e80075e8d209ab21e7eb986fbadb1714aca74d/dev/phpunit/functional/expected_output/magentom24nodb-threeway-diff.out.txt#L1-L52

norgeindian commented 2 years ago

@convenient , thanks, really cool this way. @peterjaap also included it in the GUI, so we have it both ways now. Really cool, thanks for implementing that.

convenient commented 2 years ago

Let me know how you get on @norgeindian

norgeindian commented 2 years ago

@convenient , sorry for coming back to you so late. Just started a new update and currently test the new feature. One thing, which would be awesome, would be to remove the additional line for Plugins, because there is not much to compare in a three-way diff. As far as I see, it's already not shown for queue consumer changes, but would be awesome to remove it for plugins as well. What do you think? Would you agree? Then I would be happy to provide a PR.

convenient commented 2 years ago

@norgeindian Makes sense

I've quite a large refactor going on in this #79

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/adfb92c451fc0484398878312a68fb399a2c40e7/src/Ampersand/PatchHelper/Helper/PatchOverrideValidator.php#L259-L262

I guess instead of handling plugin here we can just put a continue ?

norgeindian commented 2 years ago

@convenient , I agree. A continue should be totally fine, as I don't see any useful three-way diff here to use.

convenient commented 2 years ago

@norgeindian Are you actively working on the upgrade / still have it set up locally ?

If so do you mind

git pull
git checkout fix-issue-77-housekeeping-and-tidy-refactor-and-unit-tests
composer install
# adding the `continue;` where we discsused

This should give me a human trial run of the refactor to support unit testing and more checks in the future, as well as verifying that it fixes your threeway diff 😉 ? no worries if not

norgeindian commented 1 year ago

@convenient , sorry, the task is already done, so I currently don't have a fitting testing project. But will definitely keep it in mind and come back to you as soon as I'm doing the next update. Thanks a lot for your work here.

AmpersandHQ / ampersand-magento2-upgrade-patch-helper

Possibility to open three-file-diff viewer directly from the output #70