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

Fix composer-required plugin semantics #25

Conversation

Projects
None yet
4 participants
@hanneswernery
Copy link

commented Apr 9, 2019

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 composer vendor/ folder contains all dependencies specified in that plugin's composer.json. This is problematic since plugins may be distributed as e.g. ZIP files which ship their own vendor/ 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 adds custom/plugins as an additonal repository: https://github.com/shopware/development/blob/55cc7cb310d16e0bc614bfcfe108e0686ccbe586/composer.json#L9-L15

This allows shop administrators to composer require plugins which reside in custom/plugins into Shopware's vendor/ directory, resolving the plugins' dependencies and generating optimized classmaps in the process. However, if you try this and then run bin/console plugin:refresh, this will fail with a unique constraint violation because the PluginFinder tries to add the plugin from custom/plugins as well as from the vendor/ directory.

To fix this, this PR resolves these conflicts by always preferring plugins installed into the vendor/ folder over those installed in custom/plugins.

@@ -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()) {

This comment has been minimized.

Copy link
@janbuecker

janbuecker Apr 15, 2019

Member

@mitelg the validation is always required, isn't it? Even if it's coming from custom/plugins.

This comment has been minimized.

Copy link
@mitelg

mitelg Apr 15, 2019

Contributor

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 😉

This comment has been minimized.

Copy link
@fixpunkt

fixpunkt Apr 15, 2019

Contributor

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.

This comment has been minimized.

Copy link
@mitelg

mitelg Apr 15, 2019

Contributor

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

This comment has been minimized.

Copy link
@fixpunkt

fixpunkt Apr 15, 2019

Contributor

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?

This comment has been minimized.

Copy link
@mitelg

mitelg Apr 15, 2019

Contributor

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 😊

@janbuecker

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

This issue has been resolved by @mitelg using a little different approach which also checks non-composer installed plugins. The change can be found here d02f99a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.