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

UX: Indicate dependencies that are automatically enabled/disabled #164

Closed
klonos opened this issue Oct 29, 2021 · 4 comments · Fixed by #445
Closed

UX: Indicate dependencies that are automatically enabled/disabled #164

klonos opened this issue Oct 29, 2021 · 4 comments · Fixed by #445
Assignees

Comments

@klonos
Copy link
Member

klonos commented Oct 29, 2021

When running bee en devel_generate, devel is also enabled because it is specified as a dependency for devel_generate 👍🏼 ...that's not being communicated to the user though:

bee en devel_generate

 [✔] The 'Devel Generate' module was enabled. 

I'd like this to be like so instead:

bee en devel_generate

 [i] The 'Devel' module will also be enabled, as it is required by the 'Devel Generate' module.
 [✔] The 'Devel' module was enabled.
 [✔] The 'Devel Generate' module was enabled.

...or:

bee en devel_generate

 [i] The 'Devel' module will also be enabled, as it is required by the 'Devel Generate' module.
 [i] The 'Devel' module is already enabled.
 [✔] The 'Devel Generate' module was enabled.

Similarly when disabling modules that are dependencies for other modules:

Current behaviour:

bee dis devel

 [✔] The 'Devel' module was disabled.

What I would like to see instead is something like this instead:

bee dis devel

 [i] The 'Devel Generate' module will also be disabled, as it depends on the 'Devel' module.
 [✔] The 'Devel Generate' module was disabled.
 [✔] The 'Devel' module was disabled.

In fact, "silently" enabling/disabling module dependencies may be seen as a bug. Drush prompts for this:

drush dis devel
The following projects will be disabled: devel_generate,devel.
    Do you want to disable the projects? (y/n): y

	Success: devel_generate,devel are disabled.
drush en devel_generate
The following projects will be enabled: devel, devel_generate.
    Do you want to enable the projects? (y/n): y

	Success: module devel enabled.


	Success: module devel_generate enabled.
@klonos klonos changed the title Indicate dependencies that are automatically enabled/disabled UX: Indicate dependencies that are automatically enabled/disabled Oct 29, 2021
@yorkshire-pudding
Copy link
Collaborator

@klonos - I'm not sure how we would do this given that all the dependency processing happens within core functions module_enable() and module_disable() which have no options to report on additional modules enabled or disabled.

For disabling, we'd somehow have to check whether a module was still required by another module or not before saying it was going to be disabled.

Do you have any ideas on how to do this?

@jenlampton
Copy link
Member

Can we look at how drush handles this? Does it call module_enable() or do its own thing instead?

@elisseck
Copy link
Contributor

elisseck commented Sep 27, 2024

Can we look at how drush handles this? Does it call module_enable() or do its own thing instead?

It looks like drush builds its own list unless i'm looking at the wrong spot: https://github.com/drush-ops/drush/blob/b27e30ebabc6a2c3ce56eaeaaa7fa67db7e9ee96/commands/core/drupal/environment_7.inc#L100

I think this feature is really important for practical usability and I'm happy (and have some time available) to work on the strategy we talked about in the dev meeting today which will be:

  1. Copy what Backdrop core is doing for bee so we can get this feature in the short term (I don't think doing the calculation twice is really going to be much of a performance hit except in crazy edge cases).
  2. Open an issue against core to move the "dependent" and "dependency" checks out side of module_enable() and module_disable() so contrib modules can have those bits available, and we can stop doing the calculation twice.

elisseck added a commit to elisseck/bee that referenced this issue Sep 27, 2024
module_disable() functions, and utilize them when enabling
and disabling modules to notify the user of dependent and/or required
modules that are being disabled/enabled in addition to the named
modules.

Issue backdrop-contrib#164
@yorkshire-pudding
Copy link
Collaborator

Discussion during the Dev meeting on 2024-09-26: https://www.youtube.com/live/uSG6jpHF8dk?si=pqKKtIKEyOhKUJSj&t=760 (I've added this link above)

Thank you @elisseck for raising this and for preparing a pull request. I've added an initial comment there but will give a more detailed review shortly.

elisseck added a commit to elisseck/bee that referenced this issue Oct 10, 2024
- Pull temporary code into its own file includes/dependencies.inc
- Make use of bee_instant_message() where appropriate
- Add clarity to user message when a dependency of a dependency is disabled
- Fix code comment typo abled() -> enabled()
- Updated changelog

Issue backdrop-contrib#164
elisseck added a commit to elisseck/bee that referenced this issue Oct 10, 2024
- Pull temporary code into its own file includes/dependencies.inc
- Make use of bee_instant_message() where appropriate
- Add clarity to user message when a dependency of a dependency is disabled
- Fix code comment typo abled() -> enabled()
- Updated changelog

Issue backdrop-contrib#164
elisseck added a commit to elisseck/bee that referenced this issue Oct 10, 2024
- Pull temporary code into its own file includes/dependencies.inc
- Make use of bee_instant_message() where appropriate
- Add clarity to user message when a dependency of a dependency is disabled
- Fix code comment typo abled() -> enabled()
- Updated changelog

Issue backdrop-contrib#164
elisseck added a commit to elisseck/bee that referenced this issue Oct 10, 2024
- Pull temporary code into its own file includes/dependencies.inc
- Make use of bee_instant_message() where appropriate
- Add clarity to user message when a dependency of a dependency is disabled
- Fix code comment typo abled() -> enabled()
- Updated changelog

Issue backdrop-contrib#164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants