magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.5k stars 9.3k forks source link

Js mixins, path mappings are applied even if modules are not used but are enabled #15967

Open speedupmate opened 6 years ago

speedupmate commented 6 years ago

Js mixins and/or path mappings are applied even if modules are not used but are enabled. For example Magento 2 ships with Temando_Shipping extension , this defines mappings on few js files.

If Temando_Shipping is not configured from admin to be used on frontend , mappings still apply and affect the original behaviour that is needed when extension is not configured to be used.

Of course Temando_Shipping is just an example here , same applies to every mixin/mapping defined for any included extension in requirejs-config.js

Preconditions

  1. All Magento 2 versions

Steps to reproduce

  1. Install Magento with demo data 2.2.4 (don't configure Temando_Shipping)
  2. Add stuff to cart , reach checkout
  3. Temando_Shipping module mappings are active in checkout even if module is not enabled (not even arguing what issues it creates, but just to illustrate)

Expected result

  1. If extension is not configured to be used, we expect that it does not affect whatever is target for a mixin cause there is no need for customisation
  2. There needs to be a way to tie mixin enabling to configuration status not just enabled/disabled status of whole module. There might even be a need for more precise control on when mixins/mappings are applied by the area extension customises (by layout handler etc)

Actual result

  1. Temando_Shipping module mappings are active in checkout even if shipping method is not enabled from configuration.
  2. Dependant on developer skill or amount of methods altered this affects other extensions or even default modules to work as expected
DanielRuf commented 6 years ago

The JS files are also always loaded.

ghost commented 6 years ago

Hi @speedupmate thank you for your report. We've acknowledged the issue and added to our backlog.

DrewML commented 5 years ago

Possibly fixed by https://github.com/magento/magento2/pull/19751

speedupmate commented 5 years ago

No its not fixed by this as the issue is that module is enabled in sytem but not configured from backend to be on, enabled, configured to work. Example as Temando_Shipping is a shipping method and is only meant to be included in code when module is enabled and configured to be "on" from shipping methods.

To fix this Magento either needs 2 states of declaring a module enabled:

  1. enabled in system as bin/magento module:enable Some_Module
  2. enabled in system and enabled to work/configured from backend as a flag in config

or

Each mixin, js mapping needs to check if its module is enable before applying their changes

sivaschenko commented 3 years ago

@speedupmate I would expect that all mixins introduced by an enabled module are active.

If the mixin functionality depends on certain configuration settings (i.e enabled) the implementation should include the corresponding condition.

Please let me know what you think about that.

speedupmate commented 3 years ago

@sivaschenko and how do you see this being implemented on what level and how exactly?

You can trick requirejs-config.js by writing js conditions inside it as it is a js file after all. For example reading some state from window.context and true/false your mixin status.

However if this would be preferred way it should be documented and included by default. Then all extensions implementing mixins with "configuration level settings" should output their status to window scope or implement a feature detection some sort to make the decision when mixins are applied.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

speedupmate commented 3 years ago

haha , you mean let it rot until it fixes itself ?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

speedupmate commented 3 years ago

haha , you mean let it rot until it fixes itself ?

atIOCrON commented 3 years ago

Any update on this? A deleted user said it was added to the backlog in 2018. What's going on with this in 2021?

speedupmate commented 3 years ago

@sivaschenko this issue reported from 2018 bites hard today and 2.4.3 is extra plagued with this , see last referenced issues .

If there was a way to validate mixins against config values or coded conditions like layout nodes can would be great . Even if mistakes are continued to be made there would be a way for others to suggest or code and PR a proper solution for customers without bashing the core or 3rd party bundled closed source extensions.

For example reCaptcha is separated to lot of different modules, hardcoded cross dependencies between each other in mixin level, one mixins jquery methods (for all others in site) etc , is not enabled by default but still affects everything by default.

If all mixins are applied when module is present it tweaks the reality, defaults to unexpected state. Referenced issues show for the worst that it does so to the extent where it's just not usable out of the box without understanding why those issues happen, from where those originate and what to disable or force configure and use to avoid those.

ihor-sviziev commented 2 years ago

Hi @speedupmate, Thank you so much for your report!

My thoughts on why this issue still wasn't implemented are that it's not easy to fix.

First of all, currently, we could deploy static content on the "build system" (more info here: https://devdocs.magento.com/guides/v2.4/config-guide/deployment/). In this case, the build system doesn't have a DB connection.

Secondly, after changing store configuration, some functionality might work incorrectly after changing some configuration in the admin. Enabling the mixin is a pretty significant change - there might be cases like broken add to cart / broken checkout / etc. I'm pretty that having issues while installing a new module is better than after enabling it. You will initially see these issues and decide to fix or won't deploy that module to production.

Third, needed to re-deploy static content (see as "do another deployment") when you're changing some configuration in the admin. I'm sure it's not an expected thing.

I just listed all that above and think implementing this feature will cause more issues than benefits, and I believe that we shouldn't implement this feature at all.

What do you think?

speedupmate commented 2 years ago

@ihor-sviziev I'm thinking that this should be implemented, if config is changed then change is expected. If this is also documented it will benefit more than harm as you can find the cause by reading documentation.

If module is not configured or is disabled by default in config level it should not be enabled on frontend and included in builds.

If this can't be done on build time then mixin functionality could be extended in frontend and before applying a mixin it can seek for some optional config value . This would be a pseudo enable/disable since its still built and bundled but not applied if config does not follow. It will be still better than nothing since bundled extensions would not wrap when config is not met. Simply put there would be an option to avoid blindly overwriting defaults.

ihor-sviziev commented 2 years ago

@speedupmate, could you contact me in engcom slack to continue the discussion? Thank you!

ihor-sviziev commented 2 years ago

As discussed with @speedupmate in Slack - such a feature will be handy for many cases when some extension starts failing some JS. Good example - the bug in the bundled extension like https://github.com/magento/magento2/issues/33741 (when the extension is disabled).

Together we found a pretty simple solution:

  1. Add "enabledFlags" to global requires config (or if it's not possible - create a new variable) https://github.com/magento/magento2/blob/7c6b6365a3c099509d6f6e6c306cb1821910aab0/app/code/Magento/Theme/view/frontend/templates/page/js/require_js.phtml#L11-L13

    var require = {
    'baseUrl': 'https://example.com',
    'enabledFlags': ['Some_Extension', 'Magento_CheckoutAgreements',...]
    };
  2. Modify the mixins logic to support the following logic to support the config like this:

    config: {
        mixins: {
            'Magento_Checkout/js/action/place-order': {
                'Magento_CheckoutAgreements/js/model/place-order-mixin': {'enabled': true, 'ifconfig': 'Magento_CheckoutAgreements'}
            },
        }
    }

    https://github.com/magento/magento2/blob/7c6b6365a3c099509d6f6e6c306cb1821910aab0/lib/web/mage/requirejs/mixins.js#L139-L152

  3. Add mechanism for adding "enabled flags" using "ifconfig" (or something similar) through extensions

speedupmate commented 2 years ago

@ihor-sviziev thanks for thinking along and understanding the issue and offering simplest ways to solve this

ihor-sviziev commented 2 years ago

FYI I changed the type to a feature request related to dev experience, as it describes a new feature that will simplify debugging mixins.

PascalBrouwers commented 2 years ago

No don't, they move all feature requests to the forum where they die.

ihor-sviziev commented 2 years ago

@PascalBrouwers, so far now the feature requests are not moving to forums. You can see them there: https://github.com/magento/magento2/projects/30