New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The bin/magento setup:upgrade Command will Enable Non-Enabled modules #9516

Closed
astorm opened this Issue May 4, 2017 · 34 comments

Comments

Projects
None yet
@astorm

astorm commented May 4, 2017

Preconditions

  1. Magento installed via composer --create-project method, confirmed 2.1.3, 2.1.5, 2.1.6

Steps to reproduce

  1. Create a new module in app/code ($ pestle.phar magento2:generate:module AbcCorp Testbed 0.0.1
  2. Run php bin/magento setup:upgrade
  3. Examine app/etc/config.php

Expected result

  1. AbcCorp_Testbed should not appear in app/etc/config.php

Actual result

  1. AbcCorp_Testbed appears in app/etc/config.php, and is enabled

It's a reasonable developer expectation that setup:update should not enable modules that were not previously enabled. This bug came from real world interactions with working developers who hit this bug. This bug caused unexpected Setup Upgrade code to run, and resulted in lost real world working billable hours.

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent May 4, 2017

The reason for the current behavior is:

  • If you install Magento for the first time, you expect all the modules to be turned on by default.
  • If you purchase and install a new extension, you expect it to be installed and turned on by default.
  • If you upgrade from say 2.1 to 2.2 and there are new modules important to CE, you expect them turned on by default.

Bottom line is I think there are more use cases where you want new modules turned on by default than turned off by default. Is it in this case more of a question of "if I am half way through creating something under app/code, I don't want it turned on until I have finished, otherwise things break". That is more setup:upgrade ideally should not recognize it as a module at all (don't add to config.php until its complete). Then when its "done", then turn it on. (And no, I don't have a definition of "done" in my mind yet.)

alankent commented May 4, 2017

The reason for the current behavior is:

  • If you install Magento for the first time, you expect all the modules to be turned on by default.
  • If you purchase and install a new extension, you expect it to be installed and turned on by default.
  • If you upgrade from say 2.1 to 2.2 and there are new modules important to CE, you expect them turned on by default.

Bottom line is I think there are more use cases where you want new modules turned on by default than turned off by default. Is it in this case more of a question of "if I am half way through creating something under app/code, I don't want it turned on until I have finished, otherwise things break". That is more setup:upgrade ideally should not recognize it as a module at all (don't add to config.php until its complete). Then when its "done", then turn it on. (And no, I don't have a definition of "done" in my mind yet.)

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent May 4, 2017

To be more precise, do you think the problem is modules are being picked up and turned on by default, or is the problem that the module is being recognized as module before it is ready for use?

It would be great to get a bit better insight into the original root cause of the problem. I can see that using 'Composer require' and then having upgrade not turn on a module (you have to do extra steps) would also be confusing to people (especially for CE upgrades). So it would be helpful to understand the original problem as to why someone wanted a new module turned off. (My example was I had a half built module that did not load yet - caused bin/magento to break for me. So I can believe there is a problem here worth thinking about.)

alankent commented May 4, 2017

To be more precise, do you think the problem is modules are being picked up and turned on by default, or is the problem that the module is being recognized as module before it is ready for use?

It would be great to get a bit better insight into the original root cause of the problem. I can see that using 'Composer require' and then having upgrade not turn on a module (you have to do extra steps) would also be confusing to people (especially for CE upgrades). So it would be helpful to understand the original problem as to why someone wanted a new module turned off. (My example was I had a half built module that did not load yet - caused bin/magento to break for me. So I can believe there is a problem here worth thinking about.)

@lfolco

This comment has been minimized.

Show comment
Hide comment
@lfolco

lfolco May 4, 2017

Collaborator

I think that it's just not clear that running setup:upgrade will enable any module that happens to live in app/code that had not been explicitly enabled/disabled. I discovered this when I downloaded a module to take a look at it, put it in app/code (I had an IDE project open), and then ran setup:upgrade for a different module, and all of a sudden the downloaded module was enabled and the database scripts were run.

As a developer, I would prefer to explicitly enable/disable modules rather than have a catch-all mechanism. Or a flag on setup:upgrade --disable-auto-enable or something along those lines.

Collaborator

lfolco commented May 4, 2017

I think that it's just not clear that running setup:upgrade will enable any module that happens to live in app/code that had not been explicitly enabled/disabled. I discovered this when I downloaded a module to take a look at it, put it in app/code (I had an IDE project open), and then ran setup:upgrade for a different module, and all of a sudden the downloaded module was enabled and the database scripts were run.

As a developer, I would prefer to explicitly enable/disable modules rather than have a catch-all mechanism. Or a flag on setup:upgrade --disable-auto-enable or something along those lines.

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii May 4, 2017

It would be nice to see the following:

  • just the composer modules (placed under vendor) enabled by default (since you must've explicitly installed those via composer already, and something published through composer is more likely to be ready to use)
  • and have the local ones under app disabled until an explicit bin/magento module:enable command is executed (since these are the user's own handwritten modules, and he knows better when they're ready).

Surely, such a separation can be done?

korostii commented May 4, 2017

It would be nice to see the following:

  • just the composer modules (placed under vendor) enabled by default (since you must've explicitly installed those via composer already, and something published through composer is more likely to be ready to use)
  • and have the local ones under app disabled until an explicit bin/magento module:enable command is executed (since these are the user's own handwritten modules, and he knows better when they're ready).

Surely, such a separation can be done?

@astorm

This comment has been minimized.

Show comment
Hide comment
@astorm

astorm May 4, 2017

@alankent

  1. A user adds a module to their app/code folder but does not enable it (because they don't want it enabled)

  2. Developer does something unrelated to enabling modules.

  3. Yet, somehow, a module is enabled

That's just wrong behavior. Your system fails to honor the implicit contract of the module:enable and module:disable commands. Your system fails to honor the intentions of its users.

If you install Magento for the first time, you expect all the modules to be turned on by default.

No. I, and other users, would not expect a module that's not part of Magento to be enabled by the installer. I'd expect the installer to know which modules it needed and install them, and then potentially offer to install any extensions it sees but doesn't know about.

If you purchase and install a new extension, you expect it to be installed and turned on by default.

No. I, and other users, would expect whatever system I used to install the extension to also enabled the extension I just installed, and only that extension.

If you upgrade from say 2.1 to 2.2 and there are new modules important to CE, you expect them turned on by default.

No. I, and other users, would expect the upgrade process to look at the list of extensions currently enabled, as well as any new extensions added by 2.2, and enable them.

Those three points are just -- weird. To have made an expedient choice with some side effects like this is one thing, and a part of software development. But to double down on that choice's negative side effects, to sell that it's the right behavior, as though the problem can't be fixed?

Just weird.

astorm commented May 4, 2017

@alankent

  1. A user adds a module to their app/code folder but does not enable it (because they don't want it enabled)

  2. Developer does something unrelated to enabling modules.

  3. Yet, somehow, a module is enabled

That's just wrong behavior. Your system fails to honor the implicit contract of the module:enable and module:disable commands. Your system fails to honor the intentions of its users.

If you install Magento for the first time, you expect all the modules to be turned on by default.

No. I, and other users, would not expect a module that's not part of Magento to be enabled by the installer. I'd expect the installer to know which modules it needed and install them, and then potentially offer to install any extensions it sees but doesn't know about.

If you purchase and install a new extension, you expect it to be installed and turned on by default.

No. I, and other users, would expect whatever system I used to install the extension to also enabled the extension I just installed, and only that extension.

If you upgrade from say 2.1 to 2.2 and there are new modules important to CE, you expect them turned on by default.

No. I, and other users, would expect the upgrade process to look at the list of extensions currently enabled, as well as any new extensions added by 2.2, and enable them.

Those three points are just -- weird. To have made an expedient choice with some side effects like this is one thing, and a part of software development. But to double down on that choice's negative side effects, to sell that it's the right behavior, as though the problem can't be fixed?

Just weird.

@bvboas

This comment has been minimized.

Show comment
Hide comment
@bvboas

bvboas May 6, 2017

I think @alankent is right here.

How many times did we tell Windows to index the file we just copied from an external drive?

How many times did we install an app on a MAC by drag-n-drop and didn't expect macOS to associate an file extension?

How many times did we created an email account and went to the options to enabled it?

I don't think is reasonable to expect that when you place code inside a dir that is used as extension location, to nothing happen when you say to the software. "Please update".

I think the current behavior is the expected one. Additional options can be added to exclude packages, or similar options...

bvboas commented May 6, 2017

I think @alankent is right here.

How many times did we tell Windows to index the file we just copied from an external drive?

How many times did we install an app on a MAC by drag-n-drop and didn't expect macOS to associate an file extension?

How many times did we created an email account and went to the options to enabled it?

I don't think is reasonable to expect that when you place code inside a dir that is used as extension location, to nothing happen when you say to the software. "Please update".

I think the current behavior is the expected one. Additional options can be added to exclude packages, or similar options...

@lfolco

This comment has been minimized.

Show comment
Hide comment
@lfolco

lfolco May 7, 2017

Collaborator

Currently, the only way to make sure that non-enabled (but not disabled) modules do not run their setup scripts is to enable the module, then disable it. At the very least, it would be nice to have the option to immediately disable a module without having to enable it first (currently you get a "No modules were changed." message).

Collaborator

lfolco commented May 7, 2017

Currently, the only way to make sure that non-enabled (but not disabled) modules do not run their setup scripts is to enable the module, then disable it. At the very least, it would be nice to have the option to immediately disable a module without having to enable it first (currently you get a "No modules were changed." message).

@bh-ref

This comment has been minimized.

Show comment
Hide comment
@bh-ref

bh-ref May 16, 2017

Contributor

I encountered the same problem some time ago: #2622

We switched our workflow since then: if there is a Magento update with new modules, we run setup:upgrade on our development systems which adds the module to app/etc/env.php. If we decide to not use the module, we let the module entry there, but turn it from 1 to 0:

<?php
return array (
  'modules' => 
  array (
    ....
    'Legit_Module' => 1,
    'Unwanted_Module' => 0,
   ...
  ),
);

If we deploy this code base to the productive environments, the update / setup scripts won't be executed. We use this workflow for Magento updates and extensions we maybe want to be enabled on development systems, but not on the productive systems (so we maintain different app/etc/env.php).

But to be honest, I do not know how well this works with updates via composer, we always use the compressed archives for updating Magento.

And I totally see why developers see the current behavior of app/etc/env.php as weird. I seems it boils down to the question: who is installing modules? A technically savvy user or someone who is thankful that he has not to take more actions for the module to be enabled?

Contributor

bh-ref commented May 16, 2017

I encountered the same problem some time ago: #2622

We switched our workflow since then: if there is a Magento update with new modules, we run setup:upgrade on our development systems which adds the module to app/etc/env.php. If we decide to not use the module, we let the module entry there, but turn it from 1 to 0:

<?php
return array (
  'modules' => 
  array (
    ....
    'Legit_Module' => 1,
    'Unwanted_Module' => 0,
   ...
  ),
);

If we deploy this code base to the productive environments, the update / setup scripts won't be executed. We use this workflow for Magento updates and extensions we maybe want to be enabled on development systems, but not on the productive systems (so we maintain different app/etc/env.php).

But to be honest, I do not know how well this works with updates via composer, we always use the compressed archives for updating Magento.

And I totally see why developers see the current behavior of app/etc/env.php as weird. I seems it boils down to the question: who is installing modules? A technically savvy user or someone who is thankful that he has not to take more actions for the module to be enabled?

@rparsi-commer

This comment has been minimized.

Show comment
Hide comment
@rparsi-commer

rparsi-commer May 29, 2017

Is is possible to disable a custom module via its' etc/module.xml file?

rparsi-commer commented May 29, 2017

Is is possible to disable a custom module via its' etc/module.xml file?

@magento-engcom-team

This comment has been minimized.

Show comment
Hide comment
@magento-engcom-team

magento-engcom-team Sep 28, 2017

Contributor

@astorm, thank you for your report.
This seems to be correct Magento behavior. Please refer to the Community Forums or the Magento Stack Exchange site for advice or general discussion about this.
Otherwise you may submit Pull Request with the suggested changes.

Contributor

magento-engcom-team commented Sep 28, 2017

@astorm, thank you for your report.
This seems to be correct Magento behavior. Please refer to the Community Forums or the Magento Stack Exchange site for advice or general discussion about this.
Otherwise you may submit Pull Request with the suggested changes.

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Oct 2, 2017

This seems to be correct Magento behavior.

With all due respect, that's an overstatement. Current behavior is very far from perfect.

Please refer to ... for advice ...

IMHO telling Alan Storm (of all people) to seek an "advice" on Magento issues should be considered an insult.

Otherwise you may submit Pull Request with the suggested changes.

Make up your mind already. If you agree that a PR fixing this bug should be accepted, then the current behavior cannot be correct.

non-issue

What's that supposed to mean?
Considerning the comments above, this bug is being a real issue to some real people.
Marking it as a non-issue underminds the effort people have put into investigating possible solutions to this issue.

korostii commented Oct 2, 2017

This seems to be correct Magento behavior.

With all due respect, that's an overstatement. Current behavior is very far from perfect.

Please refer to ... for advice ...

IMHO telling Alan Storm (of all people) to seek an "advice" on Magento issues should be considered an insult.

Otherwise you may submit Pull Request with the suggested changes.

Make up your mind already. If you agree that a PR fixing this bug should be accepted, then the current behavior cannot be correct.

non-issue

What's that supposed to mean?
Considerning the comments above, this bug is being a real issue to some real people.
Marking it as a non-issue underminds the effort people have put into investigating possible solutions to this issue.

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Oct 25, 2017

Contributor

@astorm

A user adds a module to their app/code folder but does not enable it (because they don't want it enabled)
Developer does something unrelated to enabling modules.
Yet, somehow, a module is enabled

setup:upgrade IS related to enabling modules.

have the local ones under app disabled until an explicit bin/magento module:enable command is executed (since these are the user's own handwritten modules, and he knows better when they're ready).

No, thanks, I don't want to run this command manually for each module. Why even put modules not supposed to be enabled into app/code?

Whoever will work on this issue please don't change default behavior but make a configuration option for those who need another behavior.

According to my understanding there could be some interaction added to setup:upgrade command with some yellow message "New modules detected AbcCorp_Testbed, AbcCorp_Testtable, AbcCorp_Testchair. Do you wish to enable them?" and ability to enable all with one character typed or choose status one-by-one.

Contributor

orlangur commented Oct 25, 2017

@astorm

A user adds a module to their app/code folder but does not enable it (because they don't want it enabled)
Developer does something unrelated to enabling modules.
Yet, somehow, a module is enabled

setup:upgrade IS related to enabling modules.

have the local ones under app disabled until an explicit bin/magento module:enable command is executed (since these are the user's own handwritten modules, and he knows better when they're ready).

No, thanks, I don't want to run this command manually for each module. Why even put modules not supposed to be enabled into app/code?

Whoever will work on this issue please don't change default behavior but make a configuration option for those who need another behavior.

According to my understanding there could be some interaction added to setup:upgrade command with some yellow message "New modules detected AbcCorp_Testbed, AbcCorp_Testtable, AbcCorp_Testchair. Do you wish to enable them?" and ability to enable all with one character typed or choose status one-by-one.

@orlangur orlangur added feature request and removed non-issue labels Oct 25, 2017

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Oct 26, 2017

I don't want to run this command manually for each module

Why even add a bunch of modules into app/code at once?
If you do that very often, could you please provide a use case explaining why is this needed?
The only case I think of is: when you pull code from repository and don't have an app/etc/env.php file (yet) because it's been added to .gitignore.

Also, wouldn't the existing bin/magento module:enable --all option solve that case for you?

korostii commented Oct 26, 2017

I don't want to run this command manually for each module

Why even add a bunch of modules into app/code at once?
If you do that very often, could you please provide a use case explaining why is this needed?
The only case I think of is: when you pull code from repository and don't have an app/etc/env.php file (yet) because it's been added to .gitignore.

Also, wouldn't the existing bin/magento module:enable --all option solve that case for you?

@lfolco

This comment has been minimized.

Show comment
Hide comment
@lfolco

lfolco Oct 26, 2017

Collaborator

I discovered this issue when I downloaded a third party module with the intention of looking at it / evaluating it within the context of an IDE/existing Magento project. I put it in app/code when I downloaded it, and then when doing other work that involved setup:upgrade, I noticed that the module was enabled. This was not what I was expecting; I had expected that, once I looked over the code, I could then decide if I wanted to enable it or not. When working with third party code, you often want to inspect it more carefully before running it. And, it's much easier to inspect it within a Magento project where an IDE can pick up any glaring issues, etc.

Not sure if this use case warrants a feature change, but I do think it's rather unexpected, especially given that there is already a command to enable/disable modules explicitly.

Collaborator

lfolco commented Oct 26, 2017

I discovered this issue when I downloaded a third party module with the intention of looking at it / evaluating it within the context of an IDE/existing Magento project. I put it in app/code when I downloaded it, and then when doing other work that involved setup:upgrade, I noticed that the module was enabled. This was not what I was expecting; I had expected that, once I looked over the code, I could then decide if I wanted to enable it or not. When working with third party code, you often want to inspect it more carefully before running it. And, it's much easier to inspect it within a Magento project where an IDE can pick up any glaring issues, etc.

Not sure if this use case warrants a feature change, but I do think it's rather unexpected, especially given that there is already a command to enable/disable modules explicitly.

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Oct 27, 2017

Contributor

@lfolco

downloaded a third party module with the intention of looking at it / evaluating it within the context of an IDE/existing Magento project

Obviously you shouldn't do it as without intention to run it is much safer to put in separate folder where you just have code of your project plus vendors.

Current behavior of setup:upgrade is something you should be already aware of if used Magento at least a month. It is much more appropriate scenario to keep only needed modules and simply run setup:upgrade on staging after each deploy.

Some note could be added into setup:upgrade that new modules will be enabled or some flag setup:upgrade --do-not-enable-new-modules but existing behavior should not be changed to not break existing workflows.

Contributor

orlangur commented Oct 27, 2017

@lfolco

downloaded a third party module with the intention of looking at it / evaluating it within the context of an IDE/existing Magento project

Obviously you shouldn't do it as without intention to run it is much safer to put in separate folder where you just have code of your project plus vendors.

Current behavior of setup:upgrade is something you should be already aware of if used Magento at least a month. It is much more appropriate scenario to keep only needed modules and simply run setup:upgrade on staging after each deploy.

Some note could be added into setup:upgrade that new modules will be enabled or some flag setup:upgrade --do-not-enable-new-modules but existing behavior should not be changed to not break existing workflows.

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Oct 31, 2017

existing behavior should not be changed to not break existing workflows

Any sane workflow should use a list of explicit commands rather than depend on an obscure piece of undocumented functionality that might be considered a bug and removed in the future.
IMHO both employing bin/magento module:enable --all and versionizing your config.php are better options than having setup:upgrade fulfill a totally unrelated (and surprising to many) responsibility by default.

korostii commented Oct 31, 2017

existing behavior should not be changed to not break existing workflows

Any sane workflow should use a list of explicit commands rather than depend on an obscure piece of undocumented functionality that might be considered a bug and removed in the future.
IMHO both employing bin/magento module:enable --all and versionizing your config.php are better options than having setup:upgrade fulfill a totally unrelated (and surprising to many) responsibility by default.

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Oct 31, 2017

Contributor

It is absolutely clear and useful functionality. That's why default system behavior MUST NOT be changed but some switches could be introduced to use different behavior.

If it is not clear to anyone, there is no problem with documenting it.

IMHO both employing bin/magento module:enable --all and versionizing your config.php are better options than having setup:upgrade

Both require additional actions which is why current default behavior MUST NOT be spoiled by any "improvements" which reduce its usefulness.

Contributor

orlangur commented Oct 31, 2017

It is absolutely clear and useful functionality. That's why default system behavior MUST NOT be changed but some switches could be introduced to use different behavior.

If it is not clear to anyone, there is no problem with documenting it.

IMHO both employing bin/magento module:enable --all and versionizing your config.php are better options than having setup:upgrade

Both require additional actions which is why current default behavior MUST NOT be spoiled by any "improvements" which reduce its usefulness.

@lfolco

This comment has been minimized.

Show comment
Hide comment
@lfolco

lfolco Oct 31, 2017

Collaborator

It's not clear, though, as evidenced by this ticket. It doesn't say anywhere in the dev docs that this behavior occurs. There is a distinct enable/disable command, and a setup:upgrade command. There is not indication outside of experience that setup:upgrade would incorporate functionality from the enable/disable command.

I may be working on a module in app/code that I don't want to enable right away (I'm doing install scripts, etc.), so now I have to go in and disable a module that I had never enabled? This is not a clear workflow at all.

Can you give me an example of a workflow where this functionality is expected? Outside of doing core development, that is, which is the only scenario I can see where auto-enabling modules would save time.

Collaborator

lfolco commented Oct 31, 2017

It's not clear, though, as evidenced by this ticket. It doesn't say anywhere in the dev docs that this behavior occurs. There is a distinct enable/disable command, and a setup:upgrade command. There is not indication outside of experience that setup:upgrade would incorporate functionality from the enable/disable command.

I may be working on a module in app/code that I don't want to enable right away (I'm doing install scripts, etc.), so now I have to go in and disable a module that I had never enabled? This is not a clear workflow at all.

Can you give me an example of a workflow where this functionality is expected? Outside of doing core development, that is, which is the only scenario I can see where auto-enabling modules would save time.

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Oct 31, 2017

Contributor

I may be working on a module in app/code that I don't want to enable right away

Just keep it out of the code or stash such changes before running setup:upgrade. Always keep your working tree clean to avoid undesired consequences.

Useful workflow which may be documented and must be preserved:

  • update code
  • run setup:upgrade
  • result: all non-disabled modules are upgraded, new ones are enabled

We are using it all the time working on custom modules development so that there is no need to put config.php under VCS or communicate which modules were just added.

Contributor

orlangur commented Oct 31, 2017

I may be working on a module in app/code that I don't want to enable right away

Just keep it out of the code or stash such changes before running setup:upgrade. Always keep your working tree clean to avoid undesired consequences.

Useful workflow which may be documented and must be preserved:

  • update code
  • run setup:upgrade
  • result: all non-disabled modules are upgraded, new ones are enabled

We are using it all the time working on custom modules development so that there is no need to put config.php under VCS or communicate which modules were just added.

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Oct 31, 2017

We are using it all the time

Just because you personally like this workflow more doesn't mean everyone should use it.

Could you please explain what's the advantage of your approach over the "need to put config.php under VCS" approach, if you're keeping all the modules enabled at all times anyway (I mean, you won't need to disable any module locally if you prefer to "keep it out of the code or stash such changes")?

Besides, the steps you describe above will work just as fine if you simply put config.php under VCS.

korostii commented Oct 31, 2017

We are using it all the time

Just because you personally like this workflow more doesn't mean everyone should use it.

Could you please explain what's the advantage of your approach over the "need to put config.php under VCS" approach, if you're keeping all the modules enabled at all times anyway (I mean, you won't need to disable any module locally if you prefer to "keep it out of the code or stash such changes")?

Besides, the steps you describe above will work just as fine if you simply put config.php under VCS.

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Oct 31, 2017

Contributor

Many of us may like this behavior thus it should be carefully evaluated before changing. For example, template path hints in Admin UI were first introduced as a bug not existed in M1 but later it was found useful and claimed as a feature.

  1. config.php is redundant junk in current workflow
  2. leads to conflicts when stored under VCS
  3. your store may have implicit module dependencies if order is always static due to config.php
Contributor

orlangur commented Oct 31, 2017

Many of us may like this behavior thus it should be carefully evaluated before changing. For example, template path hints in Admin UI were first introduced as a bug not existed in M1 but later it was found useful and claimed as a feature.

  1. config.php is redundant junk in current workflow
  2. leads to conflicts when stored under VCS
  3. your store may have implicit module dependencies if order is always static due to config.php
@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Oct 31, 2017

config.php is redundant junk in current workflow

Except some people prefer keeping incomplete modules in VCS, disabling them via config.php.

leads to conflicts when stored under VCS
your store may have implicit module dependencies if order is always static due to config.php

IMHO These are a bit out of scope of current discussion. Please consider reopening and upvoting #8479 if you agree that current config.php mechanics needs to be improved.

Also, with the current config.php behavior I'd rather have implicit dependencies stored in a versioned config.php because otherwise you might suddenly happen to have a different load order on some of the instances (e.g. staging or production) causing issues that are hard to reproduce and identify using a different (e.g. local) instance.

korostii commented Oct 31, 2017

config.php is redundant junk in current workflow

Except some people prefer keeping incomplete modules in VCS, disabling them via config.php.

leads to conflicts when stored under VCS
your store may have implicit module dependencies if order is always static due to config.php

IMHO These are a bit out of scope of current discussion. Please consider reopening and upvoting #8479 if you agree that current config.php mechanics needs to be improved.

Also, with the current config.php behavior I'd rather have implicit dependencies stored in a versioned config.php because otherwise you might suddenly happen to have a different load order on some of the instances (e.g. staging or production) causing issues that are hard to reproduce and identify using a different (e.g. local) instance.

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Oct 31, 2017

Contributor

some of the instances (e.g. staging or production) causing issues that are hard to reproduce and identify using a different (e.g. local) instance.

No problem, just disable such modules explicitly not breaking setup:upgrade for others.

Issue you mentioned simply does not contain any appropriate criteria as I explained in it.

some of the instances (e.g. staging or production) causing issues that are hard to reproduce and identify using a different (e.g. local) instance.

It's a good thing to not rely on any particular order. In case of problem simply get a config.php to check from problematic env.

Contributor

orlangur commented Oct 31, 2017

some of the instances (e.g. staging or production) causing issues that are hard to reproduce and identify using a different (e.g. local) instance.

No problem, just disable such modules explicitly not breaking setup:upgrade for others.

Issue you mentioned simply does not contain any appropriate criteria as I explained in it.

some of the instances (e.g. staging or production) causing issues that are hard to reproduce and identify using a different (e.g. local) instance.

It's a good thing to not rely on any particular order. In case of problem simply get a config.php to check from problematic env.

@lfolco

This comment has been minimized.

Show comment
Hide comment
@lfolco

lfolco Oct 31, 2017

Collaborator

The fact that this ticket was opened a year and a half after Magento 2 was released is indicative of the fact that this behavior is not expected; the very command name (upgrade) hides the fact that it also enables.

I'm curious as to which workflow is more common in day-to-day Magento development: users who are working on multiple modules that need to be upgraded and enabled all at once, or those that are working on one at a time? This ticket has eight up votes, one down vote, so it seems that the proposed change is preferred. How is it decided to make such a feature change? Would it be decided upon submitting a pull request?

Collaborator

lfolco commented Oct 31, 2017

The fact that this ticket was opened a year and a half after Magento 2 was released is indicative of the fact that this behavior is not expected; the very command name (upgrade) hides the fact that it also enables.

I'm curious as to which workflow is more common in day-to-day Magento development: users who are working on multiple modules that need to be upgraded and enabled all at once, or those that are working on one at a time? This ticket has eight up votes, one down vote, so it seems that the proposed change is preferred. How is it decided to make such a feature change? Would it be decided upon submitting a pull request?

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Nov 1, 2017

Contributor

Votes are irrelevant here as happy users of current workflow just don't read this ticket.

I believe implementation approach should be defined prior to PR creation, will raise this topic among maintainers.

Contributor

orlangur commented Nov 1, 2017

Votes are irrelevant here as happy users of current workflow just don't read this ticket.

I believe implementation approach should be defined prior to PR creation, will raise this topic among maintainers.

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Nov 1, 2017

No problem, just disable such modules explicitly not breaking setup:upgrade for others.

setup:upgrade is irrelevant in this case. Having a versioned config.php eliminates both the need to communicate which modules have do be disabled with other members of the team and the need to disable them explicitly on the other environments.

Having config.php unversioned AND having setup:upgrade unexpectedly change module status is a recipe for getting your Magento into a broken state.
Consider a module being added, enabled just like that, and then deleted from VCS while you still have it in your local config.php
This will lead to a cryptic Setup version for module 'Bla_blabla' is not specified error message on the storefront while the bin/magento module:disable Bla_blabla command will tell you Unknown module(s): Bla_blabla forcing you to remove it from config.php manually.

Please don't think your workflow is a silver bullet which will fit everyone's needs perfectly. Because it's not.

Issue you mentioned simply does not contain any appropriate criteria as I explained in it.

Well it's certainly a better place starting point to discuss load order issues than this ticket. That was the whole point of that ticket and there were a few good points made there.

For example, one could suggest having module load order computed at runtime and cached elsewhere. That would leave the config.php with the sole purpose of tracking which modules are enabled, and it will be totally okay then for the module order in it to be changed to alphabetic. Thus storing it in VCS would have no drawbacks whatsoever.

It's a good thing to not rely on any particular order. In case of problem simply get a config.php to check from problematic env.

Again, accessing a different environment isn't any easier than having config.php versioned in the first place. Module load sequence shouldn't be environment-specific and thus there's no reason to have it excluded from VCS.

korostii commented Nov 1, 2017

No problem, just disable such modules explicitly not breaking setup:upgrade for others.

setup:upgrade is irrelevant in this case. Having a versioned config.php eliminates both the need to communicate which modules have do be disabled with other members of the team and the need to disable them explicitly on the other environments.

Having config.php unversioned AND having setup:upgrade unexpectedly change module status is a recipe for getting your Magento into a broken state.
Consider a module being added, enabled just like that, and then deleted from VCS while you still have it in your local config.php
This will lead to a cryptic Setup version for module 'Bla_blabla' is not specified error message on the storefront while the bin/magento module:disable Bla_blabla command will tell you Unknown module(s): Bla_blabla forcing you to remove it from config.php manually.

Please don't think your workflow is a silver bullet which will fit everyone's needs perfectly. Because it's not.

Issue you mentioned simply does not contain any appropriate criteria as I explained in it.

Well it's certainly a better place starting point to discuss load order issues than this ticket. That was the whole point of that ticket and there were a few good points made there.

For example, one could suggest having module load order computed at runtime and cached elsewhere. That would leave the config.php with the sole purpose of tracking which modules are enabled, and it will be totally okay then for the module order in it to be changed to alphabetic. Thus storing it in VCS would have no drawbacks whatsoever.

It's a good thing to not rely on any particular order. In case of problem simply get a config.php to check from problematic env.

Again, accessing a different environment isn't any easier than having config.php versioned in the first place. Module load sequence shouldn't be environment-specific and thus there's no reason to have it excluded from VCS.

@magento-engcom-team

This comment has been minimized.

Show comment
Hide comment
@magento-engcom-team

magento-engcom-team Dec 11, 2017

Contributor

Hi @astorm. Thank you for the report. We are moving your feature request to the special project. You can track this issue here: magento/community-features#25

Contributor

magento-engcom-team commented Dec 11, 2017

Hi @astorm. Thank you for the report. We are moving your feature request to the special project. You can track this issue here: magento/community-features#25

@astorm

This comment has been minimized.

Show comment
Hide comment
@astorm

astorm Dec 12, 2017

@magento-engcom-team That means Magento won't be changing/fixing this, correct?

astorm commented Dec 12, 2017

@magento-engcom-team That means Magento won't be changing/fixing this, correct?

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Dec 12, 2017

@astorm, well, their readme seems to confirm your guess:
https://github.com/magento-engcom/community-features/blob/master/README.md

Magento Features Development is an Initiative to Allows Community Memebers Join to Development of Magento Features

korostii commented Dec 12, 2017

@astorm, well, their readme seems to confirm your guess:
https://github.com/magento-engcom/community-features/blob/master/README.md

Magento Features Development is an Initiative to Allows Community Memebers Join to Development of Magento Features

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Dec 12, 2017

Contributor

@astorm @korostii does MSI project mean Magento is not implementing Multi-Store Inventory? :)

Contributor

orlangur commented Dec 12, 2017

@astorm @korostii does MSI project mean Magento is not implementing Multi-Store Inventory? :)

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Dec 12, 2017

Hi @orlangur,

Can tell if it's a honest question or you're just trolling.
Please keep in mind that according to the Code of Conduct, trolling is considered "unacceptable behavior" and thus I would suggest you to be "focusing on what is best for the community" instead.
I suspect that you already know that, but, in any case, MSI seems to be a separate project with a different scope of work. And it is not relevant to this discussion.
If you have any additional insider information regarding @astorm's question, please feel free to put it in the open explicitly rather vaguely hinting about it.

I, for one, don't have any insider information thus I have to resort to publicly available information (posted above) and my personal viewpoints and speculation.
IMHO, @magento-engcom-team seems to attempt to offload every non-essential task to community members. At the same time, they seem to be mostly ignoring the feedback from said community members when it comes to making decisions (please do correct me if this impression is wrong).
Like, for example, when making a decision whether some issue is considered a bug, a suggested improvement, or (as in this case, for some unclear reason) a "feature request".

To be perfectly clear, personally I think that arbitrary moving some part of the unfinished issues to a separate repository, community forum, or a closed state does little to none good to the overall project progress.

korostii commented Dec 12, 2017

Hi @orlangur,

Can tell if it's a honest question or you're just trolling.
Please keep in mind that according to the Code of Conduct, trolling is considered "unacceptable behavior" and thus I would suggest you to be "focusing on what is best for the community" instead.
I suspect that you already know that, but, in any case, MSI seems to be a separate project with a different scope of work. And it is not relevant to this discussion.
If you have any additional insider information regarding @astorm's question, please feel free to put it in the open explicitly rather vaguely hinting about it.

I, for one, don't have any insider information thus I have to resort to publicly available information (posted above) and my personal viewpoints and speculation.
IMHO, @magento-engcom-team seems to attempt to offload every non-essential task to community members. At the same time, they seem to be mostly ignoring the feedback from said community members when it comes to making decisions (please do correct me if this impression is wrong).
Like, for example, when making a decision whether some issue is considered a bug, a suggested improvement, or (as in this case, for some unclear reason) a "feature request".

To be perfectly clear, personally I think that arbitrary moving some part of the unfinished issues to a separate repository, community forum, or a closed state does little to none good to the overall project progress.

@orlangur

This comment has been minimized.

Show comment
Hide comment
@orlangur

orlangur Dec 13, 2017

Contributor

(as in this case, for some unclear reason) a "feature request"

Reason is perfectly clear. Current behavior could be documented but as original request is to change it, it is definitely a change request of currently intended behavior.

MSI will become a part of core at some point by the way.

arbitrary moving some part of the unfinished issues to a separate repository, community forum, or a closed state does little to none good to the overall project progress.

As it was explained hundreds of times, this repository is not a right place for feature requests, let's just concentrate on quality here and manage only bug reports. This new repository seems to be a place where a work by Community members on new features may be performed and managed well. Much better than moving discussions to Community Forums as to me.

Contributor

orlangur commented Dec 13, 2017

(as in this case, for some unclear reason) a "feature request"

Reason is perfectly clear. Current behavior could be documented but as original request is to change it, it is definitely a change request of currently intended behavior.

MSI will become a part of core at some point by the way.

arbitrary moving some part of the unfinished issues to a separate repository, community forum, or a closed state does little to none good to the overall project progress.

As it was explained hundreds of times, this repository is not a right place for feature requests, let's just concentrate on quality here and manage only bug reports. This new repository seems to be a place where a work by Community members on new features may be performed and managed well. Much better than moving discussions to Community Forums as to me.

@korostii

This comment has been minimized.

Show comment
Hide comment
@korostii

korostii Dec 14, 2017

Current behavior could be documented but as original request is to change it, it is definitely a change request of currently intended behavior.

This is probably a bit off-topic. I'll just say that I really don't see where are you getting the "intended" part from and how is it relevant. I mean, the feature itself is already present, effectively making this issue a "bug report" or a "suggested improvement" at best.

This new repository seems to be a place where a work by Community members on new features may be performed and managed well. Much better than moving discussions to Community Forums as to me.

Yes, of course, it is better than forums. Personally, I do not know what's the benefit over having them right here in the same repository - but you're quite right, it's certainly an improvement.

However, that wasn't the question asked by Alan. The question, as far as I understand, was whether Magento (the company) plans to put any effort into the tasks which got moved to that repository.

Again, if you have any additional insider information regarding @astorm's question, please feel free to put it in the open explicitly.

korostii commented Dec 14, 2017

Current behavior could be documented but as original request is to change it, it is definitely a change request of currently intended behavior.

This is probably a bit off-topic. I'll just say that I really don't see where are you getting the "intended" part from and how is it relevant. I mean, the feature itself is already present, effectively making this issue a "bug report" or a "suggested improvement" at best.

This new repository seems to be a place where a work by Community members on new features may be performed and managed well. Much better than moving discussions to Community Forums as to me.

Yes, of course, it is better than forums. Personally, I do not know what's the benefit over having them right here in the same repository - but you're quite right, it's certainly an improvement.

However, that wasn't the question asked by Alan. The question, as far as I understand, was whether Magento (the company) plans to put any effort into the tasks which got moved to that repository.

Again, if you have any additional insider information regarding @astorm's question, please feel free to put it in the open explicitly.

@mattwellss

This comment has been minimized.

Show comment
Hide comment
@mattwellss

mattwellss Apr 17, 2018

Since config.php can't be reliably used to disable a module, I'll suggest composer's replace as an alternative. Here's a completely hypothetical example composer.json of a "remove modules" library:

{
    "name": "remove/these-modules",
    "version": "1.0.0",
    "replace": {
        "dotmailer/dotmailer-magento2-extension": "*",
        "temando/module-shipping-m2": "*"
    }
}

This module (remove/these-modules) must be installable. I'm currently tracking it in the main repository and using a path repository, but a separate repo and vcs works just fine, too.

Once composer require remove/these-modules is run, the modules listed in replace will be removed and the new module "installed".

n.b. as shown, multiple modules can be listed in replace.

Hope this is helpful.

mattwellss commented Apr 17, 2018

Since config.php can't be reliably used to disable a module, I'll suggest composer's replace as an alternative. Here's a completely hypothetical example composer.json of a "remove modules" library:

{
    "name": "remove/these-modules",
    "version": "1.0.0",
    "replace": {
        "dotmailer/dotmailer-magento2-extension": "*",
        "temando/module-shipping-m2": "*"
    }
}

This module (remove/these-modules) must be installable. I'm currently tracking it in the main repository and using a path repository, but a separate repo and vcs works just fine, too.

Once composer require remove/these-modules is run, the modules listed in replace will be removed and the new module "installed".

n.b. as shown, multiple modules can be listed in replace.

Hope this is helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment