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
314 stars 38 forks source link

Improvements with Hyva themes #75

Closed norgeindian closed 1 year ago

norgeindian commented 1 year ago

For a while now, https://hyva.io/ is used more and more, and we use it in some projects as well. There it would be awesome, if some parts could be improved:

Do you see any way of approaching this?

convenient commented 1 year ago

All of these kinds of things are possible @norgeindian

We have a flag for --vendor-namespaces so something like --exclude-vendor-namespaces could perhaps take hyva ?

For the theme fallback currently we hook into the magento minification and simple resolvers

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/c60d335518e38e38254d246c185275cb85f864cc/src/Ampersand/PatchHelper/Helper/PatchOverrideValidator.php#L446-L463

I do not have any Hyva stores, thus the effort to develop these changes are less of a personal priority me. If you know how the file resolution works for hyva, pull requests are very welcome. I can help out in terms of testing / pointers / debugging / etc once something is "off the ground"

norgeindian commented 1 year ago

@convenient , thanks for your fast reply. I checked that now a bit more detailed in our current updates and fear, that just excluding is not enough. So the structure of hyva is the following, assuming that you have your own theme on top of it:

So any changes in luma or blank do not affect the custom theme at all, as it is only dependent on the hyva themes, which are on their side not dependent on any core theme. So what we would need is a setting to define a substitution for the core themes, so that our custom theme is checked against this one. This would not only apply for hyva, but in general for all shops, which have an independent theme included and a custom theme built on top of this. Do you see any easy way of having such a functionality with the given structure?

The fallback theme is then another thing, actually hyva specific. @Vinai, do you maybe have an idea, how we could include this setting into this helper here? This would make hyva updates a lot easier.

convenient commented 1 year ago

@norgeindian So for hyva it would be like

  1. we need to pretend that the magento themes don't exist at all (except for adminhtml)
  2. the hyva theme would not be identified as the custom theme, but would itself be the "core" theme the system is built upon?

Currently the way we grab custom themes is here

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/e52a434ca5be461ae8ebfac6d28a9e5fd7d9a6fb/src/Ampersand/PatchHelper/Helper/Magento2Instance.php#L65-L80

Perhaps we can do something sneaky like so

This would not only apply for hyva, but in general for all shops, which have an independent theme included and a custom theme built on top of this.

Do you know off the top of your head where this is defined in the configuration ? We do boot the magento instance, so we could possibly grab and work this information out

norgeindian commented 1 year ago

@convenient , exactly what you wrote. We pretend, core themes do not exist and hyva should be the "new" core theme.

Do you know off the top of your head where this is defined in the configuration ? We do boot the magento instance, so we could possibly grab and work this information out

You mean, the definition of the fallback theme? This is defined in hyva_theme_fallback/general/theme_full_path and the value is something like frontend/vendor/fallbackTheme

convenient commented 1 year ago

Right interesting. Currently the tool works without connecting to the DB which I find useful for having it in CI so I'll have a think upon this.

Is this theme ALWAYS guaranteed to exist on hyva stores? Can I string match against this code? Hyva/reset

Vinai commented 1 year ago

Happy to provide any info. Not 100% sure what is required after reading this thread.

Regarding the hyva theme detection, what we do to determine if a theme is a Hyvä theme or not is to follow the parent themes, and if that ends at Hyva/reset it's a Hyvä theme, otherwise it's not.

    public function isHyva(): bool
    {
        $theme = $this->viewDesign->getDesignTheme();
        while ($theme) {
            if (strpos($theme->getCode(), 'Hyva/') === 0) {
                return true;
            }
            $theme = $theme->getParentTheme();
        }
        return false;
    }

Regarding the theme fallback module, this is based on system configuration. I think this should be if one in regards to the upgrade helper, since any page that it refers to will then simply use a Luma theme - no special treatment necessary.

convenient commented 1 year ago

Thanks for the isHyva @Vinai :) exactly what I was wondering how to implement

convenient commented 1 year ago

Hey @norgeindian I've take an hour or so shotgun programming at this.

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/pull/80

There's still a massive TODO and question mark about how the theme fallbacking configuration you mentioned actually works. That would likely need implemented.

This is likely as far as I can push this code change myself, we don't have any Hyva sites, and setting one up for a test instance is possible but I honestly do not know when I would get the time to do that.

Anyone who wants to use that PR as the basis or inspiration for the implementation of this, that's great.

norgeindian commented 1 year ago

@convenient , awesome, really cool. I will try to test a Hyva update as soon as possible and let you know how it goes.

amenk commented 1 year ago

Is the fallback support supposed to work yet in the PR?

We have jajuma/prg and the compat module hyva-themes/magento2-jajuma-prg installed.

I belive on the left side, the hyva-themes/magento2-jajuma-prg/src/view/frontend/templates/product/list/toolbar/sorter.phtml should be checked for diffs?

Instead, I get this in the Elgentos UI:

diff

| 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                           |
amenk commented 1 year ago

Also there is this:

| WARN | Override (phtml/js/html) | vendor/magento/module-catalog/view/base/templates/product/price/tier_prices.phtml | app/design/frontend/OurCustomer/Hyvaaaaa/Magento_Catalog/templates/product/price/tier_prices.phtml |

but the template is copied from

vendor/hyva-themes/magento2-default-theme/Magento_Catalog/templates/product/price/tier_prices.phtml