Doesn't detect changes in `etc/view.xml` yet

[hostep]

Hi

I've just noticed that the tool doesn't pick up changes in the etc/view.xml file of themes.

For example: changes like these can be useful to know about when you are still using the bundled JS feature of Luma (or this one, to know that you'll need to double the image resolution for sharper images in the minicart). So you can take over these kind of changes in your custom theme.

[convenient]

This is the section of code responsible for collecting all theme files, that's the first place i'd be looking to see if this is handled.

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/062be38b6dbe2378e1d282236dc51b67f7d0f303/src/Ampersand/PatchHelper/Helper/Magento2Instance.php#L244-L294

It would need to be in that list to be a candidate

Then the closest check that could potentially match is this one https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/src/Ampersand/PatchHelper/Checks/LayoutFileXml.php but idk if that is enough seeing as they're merged?

[convenient]

@hostep would you imagine those changes would be highlighted as an INFO as in something nice to know, rather than a definitive thing that you have overridden ?

[hostep]

I think it should be a WARN type. Your attention should be drawn to it whenever it changes between Magento versions in my opinion.

[convenient]

@hostep if you or anyone else fancies picking this up i've left some TODOs around in here https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/pull/97/files

This tool does a number of checks split into two categories

WARN - Warning level items are something that you should review and often require direct code changes. Something you or a third party have customised may need adjustment or no longer be valid based on the upgraded codebase. INFO - Information level items are something that you may want to know, but there is not always direct action necessary. These items are hidden by default and exposed with --show-info.

Unless we can identify something specific in the custom code that has been overridden I'm not 100% sure it makes sense to WARN in these scenarios 🤔 we can of course tackle that a bit more later if someone picks up the PR and efforts.

[hostep]

Hmm, I think there is a misunderstanding.

Themes can and do overwrite the etc/view.xml file. So it should be a WARN and not INFO whenever that is the case and when the original file has changed. So similar to when changes in layout xml files or phtml files are detected and when you have overwritten them in your custom theme.

No?

[convenient]

I dont know what I was thinking yesterday you are totally correct. Juggling too many things and being silly i guess :)

Maybe a good target output would be

+-------+--------------------+-------------------------------------------------+-------------------------------------------------+
| Level | Type               | File                                            | To Check                                        |
+-------+--------------------+-------------------------------------------------+-------------------------------------------------+
| WARN  | Theme View Changed | vendor/magento/theme-frontend-luma/etc/view.xml | SomeTheme_One, SomeTheme_Two,SomeTheme_Three    |
+-------+--------------------+-------------------------------------------------+-------------------------------------------------+

That way we know which themes extend or fall back to the altered theme?

[hostep]

Hmmm, why not use the exact same approach as with .phtml / .html / .js / .xml files? In terms of output I mean, not sure if it's a simple code tweak or not to detect this sort of overwritten file.

So:

+-------+--------------------------+-------------------------------------------------+-----------------------------------------------+
| Level | Type                     | File                                            | To Check                                      |
+-------+--------------------------+-------------------------------------------------+-----------------------------------------------+
| WARN  | Override (phtml/js/html) | vendor/magento/theme-frontend-luma/etc/view.xml | app/design/frontend/Vendor/theme/etc/view.xml |
+-------+--------------------------+-------------------------------------------------+-----------------------------------------------+

Having a filename I can easily copy and paste is very useful instead of having to manually go to a theme directory and finding the file myself.

(I know it doesn't mention xml in the type description, but that's also the case for other layout xml files, and I don't mind it)

[convenient]

@hostep so not every theme must have an 'etc/view.xml'

This test module for example doesn't have one https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/tree/master/dev/TestVendorModule/src/theme

Is this warning only necessary when an etc/view.xml is defined in the theme?

[hostep]

Yes indeed, so the same behavior as with other potential overwritten theme files 🙂

[convenient]

Understood 👍

[hostep]

Thank you very much!!