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

Basic Extension Dependency Support #2188

Merged
merged 32 commits into from
Oct 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
c8cfdad
Add relevant exceptions for enabling/disabling
askvortsov1 May 28, 2020
e50f22b
Add helper method for getting flarum extension dependencies
askvortsov1 May 28, 2020
7937f82
Add extension dependency resolver (Kahn's algorithm), plus unit tests
askvortsov1 May 28, 2020
acc4f65
Enable extensions in proper order
askvortsov1 May 28, 2020
ec3cd0c
Fix no default when getting extension dependencies
askvortsov1 May 28, 2020
0722382
Apply fixes from StyleCI
luceos May 28, 2020
6c686bb
Use require instead of dedicated field to get extension dependencies
askvortsov1 Jul 24, 2020
787515d
Apply fixes from StyleCI
luceos Jul 24, 2020
c374baf
Fix fake extension to not fail tests and be compliant
askvortsov1 Jul 24, 2020
30ee917
Remove unnecessary import
askvortsov1 Sep 6, 2020
fec6ff3
Fix constructor, remove KnownError since we'll need a custom handler
askvortsov1 Sep 6, 2020
54a35b4
Add custom handlers for dependency exceptions
askvortsov1 Sep 6, 2020
2e4ebef
Standardize return format
askvortsov1 Sep 6, 2020
d12fe5b
Add frontend error handler for admin dashboard
askvortsov1 Sep 6, 2020
6e4b867
Fix dependentExtensions saving the wrong thing
askvortsov1 Sep 6, 2020
7b405f0
Update src/Extension/Exception/DependentExtensionsException.php
askvortsov1 Sep 6, 2020
39ca931
Update for new alerts signature
askvortsov1 Sep 24, 2020
7be3c1f
Add more tests for dependency resolution
askvortsov1 Sep 24, 2020
d32bafc
Remove dependency resolution portion
askvortsov1 Sep 26, 2020
8becd7c
Store a set of installed extension composer names instead of an assoc…
askvortsov1 Sep 26, 2020
ebe0107
Improve explanation for installedSet, put its values as true, not null
askvortsov1 Sep 30, 2020
3fa2cdd
Fix comments
askvortsov1 Oct 2, 2020
e1d6378
Get rid of the $installedSet mess, use $extensions to check what is/i…
askvortsov1 Oct 2, 2020
7acf52b
Go back to using $installedSet to store composer package names for in…
askvortsov1 Oct 2, 2020
58477b5
Code cleanup
askvortsov1 Oct 2, 2020
8923b0c
Rename `extensionDependencies` to `extensionDependencyIds` for clarity
askvortsov1 Oct 2, 2020
12e60d9
Apply fixes from StyleCI
luceos Oct 2, 2020
4ba8aba
Improve extension dependency error messages, even though they aren't …
askvortsov1 Oct 2, 2020
294a8bc
Apply fixes from StyleCI
luceos Oct 2, 2020
36f8fa0
Fix leftover `extensionDependencies`
askvortsov1 Oct 2, 2020
fef0d48
Add workaround to modal bug caused by bootstrap JS
askvortsov1 Oct 2, 2020
d65b767
Remove unnecessary changes
askvortsov1 Oct 2, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions js/src/admin/components/ExtensionsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ export default class ExtensionsPage extends Page {
url: app.forum.attribute('apiUrl') + '/extensions/' + id,
method: 'PATCH',
body: { enabled: !enabled },
errorHandler: this.onerror.bind(this),
})
.then(() => {
if (!enabled) localStorage.setItem('enabledExtension', id);
Expand All @@ -131,4 +132,23 @@ export default class ExtensionsPage extends Page {

app.modal.show(LoadingModal);
}

onerror(e) {
// We need to give the modal animation time to start; if we close the modal too early,
// it breaks the bootstrap modal library.
// TODO: This workaround should be removed when we move away from bootstrap JS for modals.
setTimeout(() => {
app.modal.close();

const error = JSON.parse(e.responseText).errors[0];

app.alerts.show(
{ type: 'error' },
app.translator.trans(`core.lib.error.${error.code}_message`, {
extension: error.extension,
extensions: error.extensions.join(', '),
})
);
}, 250);
}
}
47 changes: 47 additions & 0 deletions src/Extension/Exception/DependentExtensionsException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php

/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/

namespace Flarum\Extension\Exception;

use Exception;
use Flarum\Extension\Extension;

/**
* This exception is thrown when someone attempts to disable an extension
* that other enabled extensions depend on.
*/
class DependentExtensionsException extends Exception
{
public $extension;
public $dependent_extensions;

/**
* @param $extension: The extension we are attempting to disable.
* @param $dependent_extensions: Enabled Flarum extensions that depend on this extension.
*/
public function __construct(Extension $extension, array $dependent_extensions)
{
$this->extension = $extension;
$this->dependent_extensions = $dependent_extensions;

parent::__construct($extension->getId().' could not be disabled, because it is a dependency of: '.implode(', ', $this->getDependentExtensionIds()));
}

/**
* Get array of IDs for extensions that depend on this extension.
*
* @return array
*/
public function getDependentExtensionIds()
{
return array_map(function (Extension $extension) {
return $extension->getId();
}, $this->dependent_extensions);
}
}
34 changes: 34 additions & 0 deletions src/Extension/Exception/DependentExtensionsExceptionHandler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/

namespace Flarum\Extension\Exception;

use Flarum\Foundation\ErrorHandling\HandledError;

class DependentExtensionsExceptionHandler
{
public function handle(DependentExtensionsException $e): HandledError
clarkwinkelmann marked this conversation as resolved.
Show resolved Hide resolved
{
return (new HandledError(
$e,
'dependent_extensions',
409
))->withDetails($this->errorDetails($e));
}

protected function errorDetails(DependentExtensionsException $e): array
{
return [
[
'extension' => $e->extension->getId(),
'extensions' => $e->getDependentExtensionIds(),
]
];
}
}
47 changes: 47 additions & 0 deletions src/Extension/Exception/MissingDependenciesException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php

/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/

namespace Flarum\Extension\Exception;

use Exception;
use Flarum\Extension\Extension;

/**
* This exception is thrown when someone attempts to enable an extension
* whose Flarum extension dependencies are not all enabled.
*/
class MissingDependenciesException extends Exception
{
public $extension;
public $missing_dependencies;

/**
* @param $extension: The extension we are attempting to enable.
* @param $missing_dependencies: Extensions that this extension depends on, and are not enabled.
*/
public function __construct(Extension $extension, array $missing_dependencies = null)
{
$this->extension = $extension;
$this->missing_dependencies = $missing_dependencies;

parent::__construct($extension->getId().' could not be enabled, because it depends on: '.implode(', ', $this->getMissingDependencyIds()));
}

/**
* Get array of IDs for missing (disabled) extensions that this extension depends on.
*
* @return array
*/
public function getMissingDependencyIds()
{
return array_map(function (Extension $extension) {
return $extension->getId();
}, $this->missing_dependencies);
}
}
34 changes: 34 additions & 0 deletions src/Extension/Exception/MissingDependenciesExceptionHandler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/

namespace Flarum\Extension\Exception;

use Flarum\Foundation\ErrorHandling\HandledError;

class MissingDependenciesExceptionHandler
{
public function handle(MissingDependenciesException $e): HandledError
{
return (new HandledError(
$e,
'missing_dependencies',
409
))->withDetails($this->errorDetails($e));
}

protected function errorDetails(MissingDependenciesException $e): array
{
return [
[
'extension' => $e->extension->getId(),
'extensions' => $e->getMissingDependencyIds(),
]
];
}
}
62 changes: 53 additions & 9 deletions src/Extension/Extension.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Support\Arrayable;
use Illuminate\Support\Arr;
use Illuminate\Support\Collection;
use Illuminate\Support\Str;
use League\Flysystem\Adapter\Local;
use League\Flysystem\Filesystem;
Expand Down Expand Up @@ -51,6 +52,14 @@ class Extension implements Arrayable
'jpg' => 'image/jpeg',
];

protected static function nameToId($name)
{
list($vendor, $package) = explode('/', $name);
$package = str_replace(['flarum-ext-', 'flarum-'], '', $package);

return "$vendor-$package";
}

/**
* Unique Id of the extension.
*
Expand All @@ -60,6 +69,7 @@ class Extension implements Arrayable
* @var string
*/
protected $id;

/**
* The directory of this extension.
*
Expand All @@ -74,6 +84,13 @@ class Extension implements Arrayable
*/
protected $composerJson;

/**
* The IDs of all Flarum extensions that this extension depends on.
*
* @var string[]
*/
protected $extensionDependencyIds;

/**
* Whether the extension is installed.
*
Expand Down Expand Up @@ -104,9 +121,7 @@ public function __construct($path, $composerJson)
*/
protected function assignId()
{
list($vendor, $package) = explode('/', $this->name);
$package = str_replace(['flarum-ext-', 'flarum-'], '', $package);
$this->id = "$vendor-$package";
$this->id = static::nameToId($this->name);
}

public function extend(Container $app)
Expand Down Expand Up @@ -182,6 +197,24 @@ public function setVersion($version)
return $this;
}

/**
* Get the list of flarum extensions that this extension depends on.
*
* @param array $extensionSet: An associative array where keys are the composer package names
* of installed extensions. Used to figure out which dependencies
* are flarum extensions.
*/
public function calculateDependencies($extensionSet)
{
$this->extensionDependencyIds = (new Collection(Arr::get($this->composerJson, 'require', [])))
->keys()
->filter(function ($key) use ($extensionSet) {
return array_key_exists($key, $extensionSet);
})->map(function ($key) {
return static::nameToId($key);
})->toArray();
}

/**
* @return string
*/
Expand Down Expand Up @@ -253,6 +286,16 @@ public function getPath()
return $this->path;
}

/**
* The IDs of all Flarum extensions that this extension depends on.
*
* @return array
*/
public function getExtensionDependencyIds()
{
return $this->extensionDependencyIds;
}

private function getExtenders(): array
{
$extenderFile = $this->getExtenderFile();
Expand Down Expand Up @@ -363,12 +406,13 @@ public function migrate(Migrator $migrator, $direction = 'up')
public function toArray()
{
return (array) array_merge([
'id' => $this->getId(),
'version' => $this->getVersion(),
'path' => $this->path,
'icon' => $this->getIcon(),
'hasAssets' => $this->hasAssets(),
'hasMigrations' => $this->hasMigrations(),
'id' => $this->getId(),
'version' => $this->getVersion(),
'path' => $this->getPath(),
'icon' => $this->getIcon(),
'hasAssets' => $this->hasAssets(),
'hasMigrations' => $this->hasMigrations(),
'extensionDependencyIds' => $this->getExtensionDependencyIds(),
], $this->composerJson);
}
}
42 changes: 38 additions & 4 deletions src/Extension/ExtensionManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,18 @@ public function getExtensions()
// Composer 2.0 changes the structure of the installed.json manifest
$installed = $installed['packages'] ?? $installed;

// We calculate and store a set of composer package names for all installed Flarum extensions,
// so we know what is and isn't a flarum extension in `calculateDependencies`.
// Using keys of an associative array allows us to do these checks in constant time.
$installedSet = [];

foreach ($installed as $package) {
if (Arr::get($package, 'type') != 'flarum-extension' || empty(Arr::get($package, 'name'))) {
continue;
}

$installedSet[Arr::get($package, 'name')] = true;

$path = isset($package['install-path'])
? $this->paths->vendor.'/composer/'.$package['install-path']
: $this->paths->vendor.'/'.Arr::get($package, 'name');
Expand All @@ -101,6 +108,11 @@ public function getExtensions()

$extensions->put($extension->getId(), $extension);
}

luceos marked this conversation as resolved.
Show resolved Hide resolved
foreach ($extensions as $extension) {
$extension->calculateDependencies($installedSet);
}

$this->extensions = $extensions->sortBy(function ($extension, $name) {
return $extension->composerJsonAttribute('extra.flarum-extension.title');
});
Expand Down Expand Up @@ -133,17 +145,27 @@ public function enable($name)

$extension = $this->getExtension($name);

$this->dispatcher->dispatch(new Enabling($extension));
$missingDependencies = [];
$enabledIds = $this->getEnabled();
foreach ($extension->getExtensionDependencyIds() as $dependencyId) {
if (! in_array($dependencyId, $enabledIds)) {
$missingDependencies[] = $this->getExtension($dependencyId);
}
}

$enabled = $this->getEnabled();
if (! empty($missingDependencies)) {
throw new Exception\MissingDependenciesException($extension, $missingDependencies);
}

$enabled[] = $name;
$this->dispatcher->dispatch(new Enabling($extension));

$enabledIds[] = $name;

$this->migrate($extension);

$this->publishAssets($extension);

$this->setEnabled($enabled);
$this->setEnabled($enabledIds);

$extension->enable($this->container);

Expand All @@ -165,6 +187,18 @@ public function disable($name)

$extension = $this->getExtension($name);

$dependentExtensions = [];

foreach ($this->getEnabledExtensions() as $possibleDependent) {
if (in_array($extension->getId(), $possibleDependent->getExtensionDependencyIds())) {
$dependentExtensions[] = $possibleDependent;
}
}

if (! empty($dependentExtensions)) {
throw new Exception\DependentExtensionsException($extension, $dependentExtensions);
}

$this->dispatcher->dispatch(new Disabling($extension));

unset($enabled[$k]);
Expand Down
Loading