search

AmpersandHQ / ampersand-magento2-upgrade-patch-helper

Helper script to aid upgrading magento 2 websites by detecting overrides. Now supports third party module detections
GNU Lesser General Public License v3.0
322 stars 39 forks source link

Fix theme handling and Hyva compatibility improvements #80

Closed convenient closed 1 year ago

convenient commented 2 years ago

First change is to ensure that themes are properly checkable, not sure exactly when this regressed but likely https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/pull/50/files#diff-1677a5ff5bacd5ace1e65ee7fb17d5a6875c82d8f0e747f0df8cde8e80cc0050R508

I've captured test cases to ensure that core magento theme files are handled, as well as custom themes residing in vendor are checked.

+ | WARN  | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/templates/cart/form.phtml | app/design/frontend/Ampersand/theme/Magento_Checkout/templates/cart/form.phtml                 |
+ | WARN  | Override (phtml/js/html) | vendor/magento/module-sales/view/frontend/email/order_new.html                                         | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Sales/email/order_new.html |
+ | WARN  | Override (phtml/js/html) | vendor/magento/theme-frontend-luma/Magento_Sales/email/order_new.html                                  | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Sales/email/order_new.html |

The second change is to improve output for hyva themes.

If a template is a "hyva owned" template (it is defined in a theme with Hyva/ in the name) then we do not care that Magento had modified a template of the same path structure for any of the hyva themes or children themes. By checking if the template is defined in one of the base hyva themes, we roughly handle the case where theme fallback is in play as if a vendor/magento/some/template.phtml file is scanned and does not exist in the hyva themes, but exists in a child theme, it will still be reported.

We only need to see the hyva children themes being reported, when changes to the hyva base themes themselves occur. So we filter output, and ensure reporting is a bit like below.

- | WARN  | Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/templates/address/edit.phtml                              | vendor/hyva-themes/magento2-default-theme/Magento_Customer/templates/address/edit.phtml       | 
- | WARN  | Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/templates/address/edit.phtml                              | app/design/frontend/OurCustomer/Hyvaaaaa/Magento_Customer/templates/address/edit.phtml        | 
+ | WARN  | Override (phtml/js/html) | vendor/hyva-themes/magento2-default-theme/Magento_Customer/templates/address/edit.phtml                | app/design/frontend/OurCustomer/Hyvaaaaa/Magento_Customer/templates/address/edit.phtml        |

This works inversely also, changes in hyva themes in vendor will not be reported in non-hyva based app/code themes.

This filtering in its total form removes these kinds of entries which I believe makes sense

-| WARN  | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-hyva-theme-stub/theme/Magento_Checkout/templates/cart/form.phtml | app/design/frontend/Ampersand/theme/Magento_Checkout/templates/cart/form.phtml                                  |
-| WARN  | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-hyva-theme-stub/theme/Magento_Checkout/templates/cart/form.phtml | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/templates/cart/form.phtml          |
-| WARN  | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/templates/cart/form.phtml      | vendor/ampersand/upgrade-patch-helper-test-hyva-theme-extended/theme/Magento_Checkout/templates/cart/form.phtml |
-| WARN  | Override (phtml/js/html) | vendor/ampersand/upgrade-patch-helper-test-module/src/theme/Magento_Checkout/templates/cart/form.phtml      | vendor/ampersand/upgrade-patch-helper-test-hyva-theme-stub/theme/Magento_Checkout/templates/cart/form.phtml     |

TODO

Checklist

norgeindian commented 1 year ago

@convenient , I'm not quite sure, where this fits into, so I will add a new comment here. We are including our custom themes via composer. So they are located in some custom src folder and then included via composer. For modules, this works fine, and the correct classes are found without issues. For non-hyva-themes this worked also without issues in the past. But for hyva-themes, this somehow does not work. Do you have an idea, where to adapt that? When I take a look at Ampersand\PatchHelper\Service\GetAppCodePathFromVendorPath, this checks currently only for modules and libraries. This is why we don't get any appCodeFilepath and the canValidate check in the AnalyseCommand fails. But as far as I see that, this should fail for "normal" themes as well. So I'm not quite sure, if there is something special with the hyva theme or if you included some changes lately into master which would explain that. Like I wrote, this used to work with "normal" themes in the past. But can currently not really check, as I don't have a fitting project open. Can you think of a change, which might have leaded to this behavior or could it actually be hyva specific?

convenient commented 1 year ago

But as far as I see that, this should fail for "normal" themes as well. So I'm not quite sure, if there is something special with the hyva theme or if you included some changes lately into master which would explain that.

@norgeindian I have test coverage for vendor themes changing that seems to work okay :/

We have this theme registered like so in a vendor module https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/4e464bdc2b4a31e5b49cb149a2fe5ca1c9b2c051/dev/TestVendorModule/src/registration.php#L4-L8

There is an override in the vendor module src/theme/Magento_Checkout/templates/cart/form.phtml https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/4e464bdc2b4a31e5b49cb149a2fe5ca1c9b2c051/dev/TestVendorModule/src/theme/Magento_Checkout/templates/cart/form.phtml#L1

And it is reported in the output like so https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/4e464bdc2b4a31e5b49cb149a2fe5ca1c9b2c051/dev/phpunit/functional/expected_output/magentom24.out.txt#L16

It's entirely possible there has been some edge case regression, i was relying on my functional suite to make sure https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/pull/79 didn't break these things but it is possible that something I did not have coverage for slipped in.

To debug this what i would do is generate the patchfile with changes to the hyva theme, and pick one of the .phtml files, then run this tool with the --filter "some/filename/here.phtml" so it's easier to xdebug the process for this specific file.

convenient commented 1 year ago

also @norgeindian if you run the tool with -vvv you can share some of the output of what it says while it's trying to scan the theme files you are now seeing skipped. In case some kind of exception is being thrown

convenient commented 1 year ago

Can you think of a change, which might have leaded to this behavior or could it actually be hyva specific?

@norgeindian best guess it's been broken for a while, i had test cases which caught themes on the right hand side, but none that caught themes on the left hand side 🤦

This comment you can see I've got a fix in that should help out

amenk commented 1 year ago

patch-helper-output.txt

amenk commented 1 year ago

patch-helper-after-continue.txt

convenient commented 1 year ago

@amenk I've captured, i think, what we were working on in tests in https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/pull/83 and merged into this branch.

I also found that paths in hyva themes that changed, were being WARNd in non-hyva themes so I fixed that as well.

Can you please sync up and re-run the report? feel free to slack it to me or whatever.

amenk commented 1 year ago

patch-helper-output.txt

@convenient cool, here you are.

convenient commented 1 year ago

Thanks @amenk

I see these removed, is that expected for you?

| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar/sorter.phtml                                                  | app/design/frontend/OurCustomer/TangaSports/Jajuma_PRG/templates/product/list/toolbar/sorter.phtml                          |
| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar/sorter.phtml                                                  | app/design/frontend/OurCustomer/Hyvaaaaa/Jajuma_PRG/templates/product/list/toolbar/sorter.phtml                             |
| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar.phtml                                                         | app/design/frontend/OurCustomer/TangaSports/Jajuma_PRG/templates/product/list/toolbar.phtml                                 |
| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar.phtml                                                         | app/design/frontend/OurCustomer/Hyvaaaaa/Jajuma_PRG/templates/product/list/toolbar.phtml                                    |

I also now see a few more items in the report like this, are these new overrides you have placed or have my changes in the tool allowed more scans somehow? (this is not an exhaustive list)

| WARN  | Override (phtml/js/html) | vendor/hyva-themes/magento2-default-theme/Magento_Catalog/templates/product/list/item.phtml                                             | app/design/frontend/OurCustomer/TangaSports/Magento_Catalog/templates/product/list/item.phtml                                          |
| WARN  | Override (phtml/js/html) | vendor/hyva-themes/magento2-default-theme/Magento_Catalog/templates/product/list/item.phtml                                             | app/design/frontend/OurCustomer/Hyvaaaaa/Magento_Catalog/templates/product/list/item.phtml                                             |
| WARN  | Override (phtml/js/html) | vendor/hyva-themes/magento2-default-theme/Magento_Catalog/templates/product/slider/product-slider.phtml                                 | app/design/frontend/OurCustomer/Hyvaaaaa/Magento_Catalog/templates/product/slider/product-slider.phtml                                 |
| WARN  | Override (phtml/js/html) | vendor/hyva-themes/magento2-default-theme/Magento_Catalog/templates/product/view/addtocompare.phtml                                     | app/design/frontend/OurCustomer/Hyvaaaaa/Magento_Catalog/templates/product/view/addtocompare.phtml                                     |
amenk commented 1 year ago

I believe I did not touch the jajuma prg files yet. They seem to be module fallback cases?

Also I did not put new overrides.

convenient commented 1 year ago

@amenk Okay, so the refactor seems to have added some cases and broken the fallback mechanism. I see what i've done and can fix that.

convenient commented 1 year ago

@amenk I'd be curious how the changes in this branch #84 fare? No rush

I still need to add a test case for this, if your local instance is plugged into your database it should read the fallback information from it, otherwise it'll assume any custom local theme is a potential hyva fallback.

amenk commented 1 year ago

@convenient

I'd be curious how the changes in this branch #84 fare? No rush

I still need to add a test case for this, if your local instance is plugged into your database it should read the fallback information from it, otherwise it'll assume any custom local theme is a potential hyva fallback.

the output is identical to https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/files/10148128/patch-helper-output.txt

With both of the new branches (hyva-compat-fix-theme-fallback-scans & hyva-compat-fix-theme-fallback-scans-compartest)

convenient commented 1 year ago

@amenk 2 questions

  1. Is vendor/jajuma/prg configured as a fallback theme in hyva_theme_fallback/general/theme_full_path in core_config_data? Is hyva_theme_fallback/general/enable configured to be enabled?
  2. Is your local instance connected to a working database?

I'm curious to see how your instance fares with identifying the fallback themes here https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/65b7a70609cb7a25e4e26efc9d05d1621cfc61b3/src/Ampersand/PatchHelper/Helper/Magento2Instance.php#L183-L225

amenk commented 1 year ago
  1. nope - jajmu/prg is module, not a theme, and there is a Hyvä compat module for this, with that special fall back logic
  2. yes (also redis, otherwise I am getting an error)
convenient commented 1 year ago

nope - jajmu/prg is module, not a theme, and there is a Hyvä compat module for this, with that special fall back logic

very helpful to know will think on this

convenient commented 1 year ago

@amenk Pushed to #84 , does it show the module again?

amenk commented 1 year ago

jep, got those new lines - rest unchanged:

| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar/sorter.phtml                                                             | app/design/frontend/OurCustomer/OurSports/Jajuma_PRG/templates/product/list/toolbar/sorter.phtml                                     |
| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar/sorter.phtml                                                             | app/design/frontend/OurCustomer/Hyvaaaaa/Jajuma_PRG/templates/product/list/toolbar/sorter.phtml                                        |
| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar.phtml                                                                    | app/design/frontend/OurCustomer/OurSports/Jajuma_PRG/templates/product/list/toolbar.phtml                                            |
| WARN  | Override (phtml/js/html) | vendor/jajuma/prg/view/frontend/templates/product/list/toolbar.phtml                                                                    | app/design/frontend/OurCustomer/Hyvaaaaa/Jajuma_PRG/templates/product/list/toolbar.phtml                                               |
convenient commented 1 year ago

Perfect thank you 🙌 just gotta update my tests then get that branch merged in here cheers