Skip to content

feat: cli command for enabling or disabling an extension - #3816

Merged
imorland merged 5 commits into
mainfrom
im/extension-enable-disable-command
May 1, 2023
Merged

imorland merged 5 commits into
mainfrom
im/extension-enable-disable-command

Conversation

@imorland

@imorland imorland commented Apr 30, 2023 •

Copy link
Copy Markdown
Member

Changes proposed in this pull request:
Adds a new CLI command to enable toggling of a given extension.

Usage:

 php flarum extension:enable blomstra-turnstile
 php flarum extension:disable blomstra-turnstile

Reviewers should focus on:

Screenshot
image

image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@imorland
imorland requested a review from a team as a code owner April 30, 2023 13:10
@imorland imorland added this to the 1.8 milestone Apr 30, 2023
@imorland imorland self-assigned this Apr 30, 2023

@SychO9 SychO9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is neat! Thanks! some comments.

Comment thread framework/core/src/Extension/Console/ToggleExtensionCommand.php Outdated
Comment thread framework/core/src/Extension/Console/ToggleExtensionCommand.php Outdated

@askvortsov1 askvortsov1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we planning to eventually add some kind of "enable/disable all" to this, or would that (and bisection) be a separate command?

Comment on lines +55 to +56
switch ($enabling) {
case true:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like an if/else would suffice here? I don't see how we're going to get any more cases than that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly true, I've been using the php8 match so often lately that my brain tries to produce something similar 😆

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, pattern matching is indeed wonderful!

suspiciously drops https://ocaml.org/docs/data-types on the way out

@SychO9

SychO9 commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

Are we planning to eventually add some kind of "enable/disable all" to this, or would that (and bisection) be a separate command?

The enabl/disable all could be eventually part of this same command I think, we'd probably use an all argument or --all option.

The bisect should be its own command imo

@imorland
imorland merged commit b4f3f05 into main May 1, 2023
@imorland
imorland deleted the im/extension-enable-disable-command branch May 1, 2023 07:06
@github-actions github-actions Bot mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants