-
Notifications
You must be signed in to change notification settings - Fork 999
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
Fix composer-required plugin semantics #25
Fix composer-required plugin semantics #25
Conversation
…n finding plugins in the PluginFinder
@@ -144,7 +144,7 @@ public function installPlugin(PluginEntity $plugin, Context $shopwareContext): I | |||
// TODO NEXT-1797: Not usable with Composer 1.8, Wait for Release of Composer 2.0 | |||
if (next1797()) { | |||
$this->executor->require($plugin->getComposerName()); | |||
} else { | |||
} elseif ($plugin->isManagedByComposer()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitelg the validation is always required, isn't it? Even if it's coming from custom/plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is. we tried to validate all plugins with the "require" definition from the composer.json file. But @fixpunkt and I figured out some issues on the dev-days and the whole validation needs some optimisation 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When installing a plugin from custom/plugins
, validating that plugin means that it cannot require any extra libraries and ship those in its own vendor/
folder.
With this validation in place for plugins from custom/plugins
, for example, we cannot distribute any shipping integrations as ZIPs over the store, since they need to bundle https://github.com/VIISON/AddressSplitting and NEXT-1797 likely won't be active for the initial release.
So I'd disagree, validating requirements like this is only an option if we can be certain the plugin was actually composer require
'd, which is not the case for plugins which were installed into custom/plugins
, e.g. from the store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but do we need an additional validation if the plugin was installed via composer? on "require" composer will validate and resolve all requirements.
besides that, the validator still needs to consider if a required plugin is installed and active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but do we need an additional validation if the plugin was installed via composer? on "require" composer will validate and resolve all requirements.
No, other than the yet-to-be-implemented validation you already mentioned. But that's not the issue at hand: Validating plugins which are not required using composer fails their installation if there's anything other than shopware/platform
or its transitive dependencies in their require
section.
So yeah, you're right: The call to $this->requirementValidator->validateRequirements()
should probably never happen unless NEXT-1797
is active, because:
- For non-composer-managed plugins in
custom/plugins
, it will prevent legitimate use cases (shipping extra libraries), and - For composer-managed plugins, it won't do anything that is not already done by composer.
So the correct solution would be to remove the else
branch entirely, wouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the correct solution would be to remove the
else
branch entirely, wouldn't it?
no, because then, we would have no validation at all 😃
I just started a ticket on plugin requirements check, so I will consider your cases and ours, so that all plugins should be checked correct 😊
This PR does two things.
(1)
When installing a plugin to the
custom/plugins
directory, Shopware will then try to verify that Shopware's main composervendor/
folder contains all dependencies specified in that plugin'scomposer.json
. This is problematic since plugins may be distributed as e.g. ZIP files which ship their ownvendor/
directory, which may include additional libraries. In this case, even though the plugin ships its own dependencies, Shopware will refuse to install the plugin.This PR fixes this issue by only validating composer dependencies for plugins which are
managedByComposer
. This way, Shopware validates plugin dependencies if and only if the plugin is installed into the Shopware installation using composer.(2)
shopware/development's
composer.json
addscustom/plugins
as an additonal repository: https://github.com/shopware/development/blob/55cc7cb310d16e0bc614bfcfe108e0686ccbe586/composer.json#L9-L15This allows shop administrators to
composer require
plugins which reside incustom/plugins
into Shopware'svendor/
directory, resolving the plugins' dependencies and generating optimized classmaps in the process. However, if you try this and then runbin/console plugin:refresh
, this will fail with a unique constraint violation because thePluginFinder
tries to add the plugin fromcustom/plugins
as well as from thevendor/
directory.To fix this, this PR resolves these conflicts by always preferring plugins installed into the
vendor/
folder over those installed incustom/plugins
.