Enabling/disabling extensions sometimes causes crash

[askvortsov1]

Bug Report

Current Behavior
Introduced by #2579.
Whenever we enable/disable an extension, we now re-run dependency resolution. We do this by pulling in all enabled extension IDs (from settings), mapping that to the extension objects (via getExtension()), and passing that to the algorithm.

However, that enabled ID list is just a list stored in settings, and does not update when extensions are removed, so some entries passed to the algorithm might be null, which causes a crash.

Steps to Reproduce

1. Uninstall an enabled extension via composer
2. Try to enable/disable any other extension
3. It crashes.

Possible Solution
The way I see it, we have 2 options:

1. Add an extra array_filter in the setEnabled method so that extensions that don't exist aren't set to enabled. This prevents the bug, but it won't remove extensions from the enabled list on composer remove until some other extension is toggled
2. In getExtensions, check if any keys under $enabledIds don't correspond to an extension. If so, filter out the nonexistent ones and trigger setEnabled. This would close Allow optional dependency / one-of-several dependency requirements to control extension boot order #1318 without the need for any composer scripts (a big positive) but would add extremely minimal overhead to boot. I think this is the better approach.

[SychO9]

Any reason why not having composer scripts instead is a better idea ? I don't feel comfortable having to resolve extension order every time on boot, a composer script sounds cleaner and better, we'd have a console command, something like php flarum extension:check and add that to flarum/flarum 's composer.json, not only will admins be able to manually run that, but it will be a pretty simple command without much code added.

I would be in favour of going with option 1 and adding the composer scripts next release, I can publish a branch for that as well, used it to test the composer scripts in this scenario.

[askvortsov1]

I don't feel comfortable having to resolve extension order every time on boot

But we wouldn't be doing that? On every boot, we'd be adding a check over the current enabled keys to ensure that they are all present. The set of extensions is already there from the previous step, so all we're adding is a simple loop over ~10-200 strings, each of which are fairly short. And since it's so small, that data's probably sitting in L1 cache anyway. There's no way something that small is a bottleneck. Dependency resolution would only be executed when there's a discrepancy.

And on that note, I think we might be interpreting dependency resolution as this big scary thing because we're used to composer. but composer is a mathematical juggernaut that works with graphs magnitudes larger and more complex than our measly extension dependencies. Kahn's algorithm (the implementation we use) runs in linear time over number of extensions + number of extension dependencies; its runtime is absolutely insignificant.

[SychO9]

🤦🏼‍♂️ sorry my bad, I didn't pay close attention there, I just assumed so.
It will only run when extension packages have been removed, which is very very (very) few times, never mind then.

And on that note, I think we might be interpreting dependency resolution as this big scary thing because we're used to composer. but composer is a mathematical juggernaut that works with graphs magnitudes larger and more complex than our measly extension dependencies. Kahn's algorithm (the implementation we use) runs in linear time over number of extensions + number of extension dependencies; its runtime is absolutely insignificant.

Not exactly, it's just that there is a difference between running code when composer commands are run, and code on boot, while the algorithm is fast, in my perspective it would still not be clean running it every time (which is not the case anyway).