Skip to content
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

Remove toggle:modules command #4365

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Remove toggle:modules command #4365

merged 1 commit into from
Mar 11, 2021

Conversation

danepowell
Copy link
Contributor

@danepowell danepowell commented Mar 10, 2021

Motivation

The toggle:modules command was added early in the Drupal 8 lifecycle when the config management ecosystem was still pretty immature. Nowadays there are much better alternatives for managing modules on a per-environment basis (namely, Config Split), and in fact modifying modules outside of that CM system is likely to cause conflicts.

Proposed changes

Remove toggle:modules.

Merge requirements

  • Major change, Minor change, Bug, Enhancement, and/or Chore label applied
  • Manual testing by a reviewer

@danepowell danepowell added the Major change Requires change record (often BC-breaking) label Mar 10, 2021
@danepowell danepowell changed the title DX-3041: Remove toggle:modules command Remove toggle:modules command Mar 10, 2021
@danepowell danepowell merged commit a5ac003 into acquia:main Mar 11, 2021
@dpagini
Copy link
Contributor

dpagini commented Mar 12, 2021

As a Major change I assume this wouldn't be published in a release until a 13 branch, is that right?

I have projects that don't use config management from BLT (you can choose "none" as your cm option), so in that case "config:split" is not an option to use. I realize this makes sense for a majority of sites that can easily just config import/export for workflow, but for running larger multi-sites that's not an ideal option. Personally I would like to see this option stay in BLT. Are there stats you have just saying that most projects don't use this? J/w.

@danepowell
Copy link
Contributor Author

Correct, it'll be a 13 release.

Yes, this is one of the lesser used features of BLT, and just looking at it from a prescriptive point of view, a lot of discussion indicated that config split was a superior alternative.

If you're interested in continuing to use the functionality, I'd imagine it could be rolled into a plugin pretty easily. The command itself can be ported straight to a plugin, and then you just need to invoke it as part of the build steps, which can probably be done with hooks. If not, we can consider adding a hook.

@shelane
Copy link
Contributor

shelane commented Mar 13, 2021

Yes, I would want it as a hook and plug-in. I use config split for different multi sites. You add environments on top of that and it’s hard to manage.

@marvil07-adapt
Copy link
Contributor

@danepowell , @shelane : I am wondering if this was moved into a plugin.
I have not been able to find it on the acquia/ namespace in github.
Also, if not there, which blt hook/interaction point is recommended to use for this?

@shelane
Copy link
Contributor

shelane commented Jul 30, 2021

I haven't seen it. I haven't updated to BLT 13 because of it. I have a multisite that already has config split for a few of the sites. I don't want to add a split for environments too. Usually it's to turn off modules either in local (SAML, acquia connector) or in prod (views UI, field UI, etc). If Acquia doesn't put it in their space as a plugin, I'll have to add it to my custom commands.

@danepowell
Copy link
Contributor Author

Sorry, Acquia does not plan to make toggle:modules available as a plugin.

@marvil07-adapt
Copy link
Contributor

@danepowell , @shelane : Thanks for the feedback!

@shelane
Copy link
Contributor

shelane commented Aug 25, 2021

In case anyone else is like me, here is the plugin moved into my github space:
https://github.com/shelane/toggle-modules/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major change Requires change record (often BC-breaking)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants