Basic Extension Dependency Support #2188

askvortsov1 · 2020-05-28T20:09:04Z

Refs #2093
Refs #1318

Changes proposed in this pull request:

Don't enable an extension if its dependencies are not enabled
Don't disable an extension if its dependencies are not disabled

Confirmed

Backend changes: tests are green (run composer test).
Frontend changes: tested locally

Screenshots
Error when disabling with dependent: https://i.imgur.com/X2npKFt.png
Error when enabling with missing dependencies: https://i.imgur.com/rioQMXc.png

Lang English PR: flarum/lang-english#172

luceos

I left some comments. But I'd like to discuss the need or maybe the implementation first. This has to be looked at by @franzliedke.

src/Extension/Exception/DependentExtensionsException.php

franzliedke · 2020-06-03T21:28:03Z

Why would we need a separate field for declaring extensions we depend on? Composer already has require... 🤔

Anyway, I won't look at this in more detail until we've made more progress with the frontend rewrite, sorry.

askvortsov1 · 2020-06-03T23:20:54Z

Why would we need a separate field for declaring extensions we depend on? Composer already has require ... 🤔

Composers require doesn't tell us what is and isn't a flarum extension, so if something isn't installed, we don't know if it's needed. Also, we'd need to do a lot more unnecessary computations to scan every extensions composer json whenever we add an extension. Overall this will just result in a clearer codebase imo.

Anyway, I won't look at this in more detail until we've made more progress with the frontend rewrite, sorry.

Sorry I should have made it clearer in the description: this is very much a "I wanna code something useful but don't want to add to Franz's workload so I'm gonna explore this cool idea" PR.

franzliedke · 2020-06-03T23:35:16Z

Composers require doesn't tell us what is and isn't a flarum extension, so if something isn't installed, we don't know if it's needed. Also, we'd need to do a lot more unnecessary computations to scan every extensions composer json whenever we add an extension.

Ah, but we have the information about all Composer-installed packages in memory already, just before the Extension objects are instantiated. That includes require keys as well as the type key (which needs to be checked to equal flarum-extension).

askvortsov1 · 2020-06-03T23:38:27Z

I see your point, I still think the code would be a bit messy. But let's table this for now, and revisit after frontend / extenders?

askvortsov1 · 2020-07-24T19:11:47Z

It now uses dependencies from require, and I was able to do it without significantly messing up big O complexity or code convolution!

luceos · 2020-08-02T09:14:35Z

What's the memory footprint increase of this? I'm not going to approve this PR unless we know the downside in terms of performance. Having said so, I'm not sure this is the right approach. This code is executed on each request, even though it is only needed during the installation of an extension. I would vote to move this to a composer lifecycle hook (post update/install cmd) instead if possible. That would be my first attempt.

askvortsov1 · 2020-08-02T14:25:13Z

The process for figuring out which extensions are dependent on which has an O(n) time complexity, and a O(n) space complexity (for creating the hash table of php package names to composer.json files, ie $associativeInstalled).

The process of checking whether dependencies are violated have an O(n * m) time compexity (n being number of extensions, m being average number of dependencies per extension). This is, however, inconsequential as it's only run when enabling / disabling an extension, not with every request.

The resolution of extension dependency order has an O(n + v) time complexity (as it's just Kahn's algorithm), with n being the number of extensions, and v being the number of vertices in the dependency graph (ie the sum of each extension's number of dependencies).

None of these are especially high, and all of these scale well. That being said, this code doesn't just check for whether something should be enabled/disabled, but also ensures that extensions are booted in the proper order, every time. That's why I think it should run on every request.

Also, does PHP really boot up the entire system on every request? There isn't a way to boot the application and have it await requests as they come in as a process running in the background?

tankerkiller125 · 2020-08-02T14:28:24Z

@askvortsov1 Standard PHP does not have a way to keep the application running, there are of course PHP work arounds for this using special libraries and stuff but that's not for Flarum to get into. Of course any production environment will have OPCache or APC cache enabled which does store the compiled PHP files so they execute faster. Once PHP 8 is out we'll also get to see how JIT does with performance.

clarkwinkelmann

I see a bunch of unrelated commits which I guess come from different PRs. It doesn't help to figure out what's part of this PR... Two things below.

js/src/admin/components/ExtensionsPage.js

clarkwinkelmann · 2020-09-07T01:00:12Z

src/Extension/ExtensionManager.php

@@ -296,7 +338,9 @@ public function getEnabledExtensions()
     */
    public function extend(Container $app)
    {
-        foreach ($this->getEnabledExtensions() as $extension) {
+        $enabled = static::resolveExtensionOrder($this->getEnabledExtensions());


What about doing this whenever an extension is enabled, and save the new order in the database? That way there's no need to do any computation when Flarum boots.

I suppose we could... But this way, it'll always be right (even if someone messes with the database), and decouples the storage of enabled extensions from the order in which they're booted. Plus, topological sort runs really fast, it'd hardly be a bottleneck compared to other aspects of the site.

I'm going to have to agree with Sasha here, although it's rare that the database gets modified directly it does happen sometimes. especially during some of our more complex troubleshooting processes.

And I am going to agree with @clarkwinkelmann here. This is all superfluous code that, by itself, might not seriously increase loading times (first-byte) of our forum. But each and every of these kind of decisions will impede performance over time. This scenario, with this code, is especially true with forums that have a ton of extensions - and trust me that will sadly be the reality.

Honestly I would even make this a post update/install hook by saving some kind of order map of all installed extensions to a file or the database. Then during extension enable/disable retrieve this order and store the order of enabled extensions to the database. If anyone decides to manually override that order from the database, they have good reason to do so and don't particularly would hope to see something override that order magically..

Further review into this, I'm going to agree with @luceos and @clarkwinkelmann, I also agree with luceos idea of making this a post update/install hook for composer to store the order map. Further the database edits by people are incredibly rare and if someone did do it on accident (or purpose) we could simply inform them to run the same command that composer does to resolve it.

I spent some more time thinking about this, and just the dependency checks on enabling / disabling will be a HUGE improvement over what we have now (and much, much simpler to review). And that should also result in a working order.

I'll remove the algorithms stuff and move it over to the extension management commands PR (we'll need it if we're trying to enable/disable extensions in a working order). No need to get everything done in a single PR.

Are you saying this PR will only contain logic to check whether depending extensions are installed? Sounds like a smart move. The composer hook should be quite simple too, but needs some work.

Yep! That way, we can split this into separate PRs and have the composer hook later.

As noted earlier somewhere, due to our current system, if we have this check in place before enabling/disabling extensions, that'll prevent the vast majority of issues (since load order currently depends on order of enabling, and this way, you won't be able to enable anything.

The composer hook stuff would be in a separate PR, definitely not aiming for beta 14.

askvortsov1 · 2020-09-07T05:17:26Z

I see a bunch of unrelated commits which I guess come from different PRs. It doesn't help to figure out what's part of this PR...

Yeah that was a messy rebase... sorry about that. All cleaned up now!

…iative array to their package.jsons to save memory

…sn't a flarum extension instead.

…stalled extensions

changing key in collection is not the way to go

tankerkiller125

Other than the note from luceos I'm good with these changes

luceos · 2020-10-02T19:59:53Z

We just discovered on discord that the change to use the full package name as key is a breaking change and will impact the getExtension method amongst other. So reverted my approval.

- Make extensionDependencies protected, call through encapsulated method - The dependency exceptions now store arrays of extensions, not arrays of extension ids. - Improve documentation

[ci skip] [skip ci]

clarkwinkelmann

I see I have been mentioned previously, but I can't find the message that was notified by email. In the current state of the PR, I'm not seeing any issue with other extensions.

I was about to comment on the enabled**Ids` thing, but I see a commit was just pushed fixing it.

src/Extension/Exception/DependentExtensionsExceptionHandler.php

…shown to users

[ci skip] [skip ci]

clarkwinkelmann · 2020-10-02T20:38:04Z

When the commits stop flowing in I might approve 😛

clarkwinkelmann

I shared some ideas regarding the error message on Discord, suggesting to maybe show title together with ID. But the current implementation is fine for the time being.

askvortsov1 requested review from franzliedke and luceos May 28, 2020 20:09

askvortsov1 force-pushed the as/extension_dependencies branch from ac7cf99 to f19bbd6 Compare May 28, 2020 20:30

askvortsov1 mentioned this pull request May 31, 2020

Add expose loader and expose sortable to window flarum/tags#85

Closed

luceos requested changes Jun 3, 2020

View reviewed changes

src/Extension/Exception/DependentExtensionsException.php Outdated Show resolved Hide resolved

src/Extension/Exception/DependentExtensionsException.php Outdated Show resolved Hide resolved

luceos marked this pull request as draft June 4, 2020 13:36

askvortsov1 force-pushed the as/extension_dependencies branch from 4cda9bb to b47a013 Compare July 24, 2020 19:09

askvortsov1 force-pushed the as/extension_dependencies branch from 1f4266e to 41263c0 Compare July 24, 2020 19:22

askvortsov1 marked this pull request as ready for review July 24, 2020 19:26

askvortsov1 force-pushed the as/extension_dependencies branch 2 times, most recently from 7825b50 to a1f2e84 Compare September 6, 2020 21:33

askvortsov1 requested review from luceos and clarkwinkelmann September 6, 2020 22:03

askvortsov1 changed the base branch from master to mithril-2-update September 6, 2020 22:08

askvortsov1 added this to the 0.1.0-beta.14 milestone Sep 6, 2020

askvortsov1 mentioned this pull request Sep 6, 2020

Add extension dependency error translations flarum/lang-english#172

Merged

clarkwinkelmann reviewed Sep 7, 2020

View reviewed changes

askvortsov1 force-pushed the as/extension_dependencies branch from cca7d1f to c73df92 Compare September 7, 2020 05:15

askvortsov1 requested a review from clarkwinkelmann September 7, 2020 05:17

franzliedke force-pushed the mithril-2-update branch 2 times, most recently from 8c63ff2 to 161cd57 Compare September 18, 2020 22:52

askvortsov1 added 3 commits September 30, 2020 16:14

Remove dependency resolution portion

d32bafc

Store a set of installed extension composer names instead of an assoc…

8becd7c

…iative array to their package.jsons to save memory

Improve explanation for installedSet, put its values as true, not null

ebe0107

askvortsov1 force-pushed the as/extension_dependencies branch from ff3f9e9 to ebe0107 Compare September 30, 2020 20:14

Fix comments

3fa2cdd

askvortsov1 requested a review from luceos October 2, 2020 19:05

luceos approved these changes Oct 2, 2020

View reviewed changes

Get rid of the $installedSet mess, use $extensions to check what is/i…

e1d6378

…sn't a flarum extension instead.

luceos previously approved these changes Oct 2, 2020

View reviewed changes

Go back to using $installedSet to store composer package names for in…

7acf52b

…stalled extensions

tankerkiller125 approved these changes Oct 2, 2020

View reviewed changes

askvortsov1 and others added 3 commits October 2, 2020 16:22

Code cleanup

58477b5

- Make extensionDependencies protected, call through encapsulated method - The dependency exceptions now store arrays of extensions, not arrays of extension ids. - Improve documentation

Rename extensionDependencies to extensionDependencyIds for clarity

8923b0c

Apply fixes from StyleCI

12e60d9

[ci skip] [skip ci]

clarkwinkelmann requested changes Oct 2, 2020

View reviewed changes

src/Extension/Exception/DependentExtensionsExceptionHandler.php Show resolved Hide resolved

askvortsov1 and others added 2 commits October 2, 2020 16:31

Improve extension dependency error messages, even though they aren't …

4ba8aba

…shown to users

Apply fixes from StyleCI

294a8bc

[ci skip] [skip ci]

askvortsov1 added 3 commits October 2, 2020 16:40

Fix leftover extensionDependencies

36f8fa0

Add workaround to modal bug caused by bootstrap JS

fef0d48

Remove unnecessary changes

d65b767

askvortsov1 requested review from clarkwinkelmann and luceos October 2, 2020 21:18

clarkwinkelmann approved these changes Oct 2, 2020

View reviewed changes

askvortsov1 merged commit 84d14f4 into master Oct 2, 2020

askvortsov1 deleted the as/extension_dependencies branch October 2, 2020 21:54

askvortsov1 mentioned this pull request Oct 2, 2020

Show extension titles instead of IDs for dependency error messages #2341

Closed

askvortsov1 mentioned this pull request Oct 26, 2020

AdminUX Overhaul #2409

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic Extension Dependency Support #2188

Basic Extension Dependency Support #2188

askvortsov1 commented May 28, 2020 •

edited

Loading

luceos left a comment

franzliedke commented Jun 3, 2020

askvortsov1 commented Jun 3, 2020

franzliedke commented Jun 3, 2020

askvortsov1 commented Jun 3, 2020

askvortsov1 commented Jul 24, 2020

luceos commented Aug 2, 2020

askvortsov1 commented Aug 2, 2020

tankerkiller125 commented Aug 2, 2020

clarkwinkelmann left a comment •

edited

Loading

clarkwinkelmann Sep 7, 2020

askvortsov1 Sep 7, 2020

tankerkiller125 Sep 7, 2020

luceos Sep 26, 2020

tankerkiller125 Sep 26, 2020

askvortsov1 Sep 26, 2020

luceos Sep 28, 2020

askvortsov1 Sep 28, 2020

askvortsov1 commented Sep 7, 2020

tankerkiller125 left a comment

luceos commented Oct 2, 2020

clarkwinkelmann left a comment

clarkwinkelmann commented Oct 2, 2020

clarkwinkelmann left a comment

Basic Extension Dependency Support #2188

Basic Extension Dependency Support #2188

Conversation

askvortsov1 commented May 28, 2020 • edited Loading

luceos left a comment

Choose a reason for hiding this comment

franzliedke commented Jun 3, 2020

askvortsov1 commented Jun 3, 2020

franzliedke commented Jun 3, 2020

askvortsov1 commented Jun 3, 2020

askvortsov1 commented Jul 24, 2020

luceos commented Aug 2, 2020

askvortsov1 commented Aug 2, 2020

tankerkiller125 commented Aug 2, 2020

clarkwinkelmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 commented Sep 7, 2020

tankerkiller125 left a comment

Choose a reason for hiding this comment

luceos commented Oct 2, 2020

clarkwinkelmann left a comment

Choose a reason for hiding this comment

clarkwinkelmann commented Oct 2, 2020

clarkwinkelmann left a comment

Choose a reason for hiding this comment

askvortsov1 commented May 28, 2020 •

edited

Loading

clarkwinkelmann left a comment •

edited

Loading