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

Optional Dependencies #2579

Merged
merged 7 commits into from
Feb 21, 2021
Merged

Optional Dependencies #2579

merged 7 commits into from
Feb 21, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

Refs #1318

Changes proposed in this pull request:

Reviewers should focus on:

  • I have mixed feelings about the name "optional dependencies". Maybe something like "bootBeforeTheseExtensions" would be better? But that's very verbose and would make for a clunky getter method name.

Confirmed

- [ ] Frontend changes: tested on a local Flarum installation.

  • Backend changes: tests are green (run composer test).

Required changes:

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

@askvortsov1 askvortsov1 changed the title As/optional dependencies Optional Dependencies Jan 28, 2021
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Code looks good!

src/Extension/ExtensionManager.php Show resolved Hide resolved
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

So I noticed an odd behaviour,

I created two dummy extensions that have flarum/tags as an optional dependency, I enabled both of them and observed the enabled extensions list order, looked fine and logical. I then proceeded to disable the tags extension, being an optional dependency it should not affect the other two extensions, however everytime I tried doing so one of the extensions (not always the same one) gets disabled.

Tracked it down to an issue in the extension order resolution algorithm.

src/Extension/ExtensionManager.php Outdated Show resolved Hide resolved
Copy link
Contributor

@KyrneDev KyrneDev left a comment

Choose a reason for hiding this comment

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

Can confirm the bug reported by @SychO9 is fixed, works well locally!

@askvortsov1 askvortsov1 merged commit fa10d79 into master Feb 21, 2021
@askvortsov1 askvortsov1 deleted the as/optional-dependencies branch February 21, 2021 18:49
@askvortsov1 askvortsov1 added this to the 0.1.0-beta.16 milestone Feb 21, 2021
askvortsov1 added a commit that referenced this pull request Feb 21, 2021
* Add and calculate optional dependencies
* Add extension dependency resolver (Kahn's algorithm), plus unit tests
* Resolve extension dependency on enable/disable
@askvortsov1 askvortsov1 removed this from the 0.1.0-beta.16 milestone Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants