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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #8065: Load plugins in consistent and deterministic order. #8070

Closed
wants to merge 1 commit into from

Conversation

@danepowell
Copy link
Contributor

danepowell commented Mar 29, 2019

Fixes #8065

Composer currently loads plugins in a different order depending on operation and state.

During an update, or initial install, it will activate plugins according to their dependency tree, which is IMO the expected behavior. That way, if one plugin depends on another, the dependent plugin always gets loaded first.

During subsequent operations, Composer activates plugins in alphabetical order as they are read from installed.json. This is bad because now dependencies are not necessarily available in time, causing a lot of issues for downstream projects, especially those depending on Composer Installers and starting with "A" or "B" 馃槃 acquia/blt#3148

I think it's better that installed.json is ordered according to the dependency tree, so plugins are loaded in the correct order.

Of course this also reverts #6696 which isn't ideal. But I think avoiding race conditions is important. I'm surprised people are committing and reviewing file diffs for installed.json in the first place. That should really be done by a CI tool, I've never committed a vendor directory to Git.

@danepowell

This comment has been minimized.

Copy link
Contributor Author

danepowell commented Mar 29, 2019

I don't think the test failure is my fault?

@johnoltman65

This comment has been minimized.

Copy link

johnoltman65 commented Mar 29, 2019

Thanks @danepowell 馃憤 I agree on the dependency ordering and hope someone can merge this asap. 馃檹

@legoktm

This comment has been minimized.

Copy link
Contributor

legoktm commented Apr 1, 2019

Obviously, I would not rather revert #6696. I agree with you that making the behavior consistent and deterministic is important, since that's actually what #6696 did...just in a different way. Can we sort the installed.json file a fully consistent/deterministic order that works for plugins too? alphasorting isn't important, just that it is deterministically sorted and not randomly changing.

@danepowell

This comment has been minimized.

Copy link
Contributor Author

danepowell commented Apr 1, 2019

I would still consider it deterministic if you merge this PR (reverting #6696). The order will be determined by the dependency tree. And that's exactly how it should be in order to avoid issues with dependencies between plugins. Sorting alphabetically might "feel" more deterministic, but it's really not. Unless I'm missing something?

Note that composer.lock will still be alphabetically sorted after this. And that file is much more likely to be committed to VCS (and thus reviewed by humans), so I feel like this isn't a bad compromise.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Apr 5, 2019

This doesn't really look like the best way to fix this. There is

protected function sortPackageMap(array $packageMap)
which sorts package by dependency weight, which isn't 100% reliable IIRC but will fix most of these cases. We could sort plugins accordingly before loading them.

@Seldaek Seldaek closed this Apr 5, 2019
@danepowell

This comment has been minimized.

Copy link
Contributor Author

danepowell commented Apr 6, 2019

@Seldaek I'm not sure how to do what you propose, and would appreciate some guidance.

First of all, just to confirm, you want to sort the packages each time they are loaded (which might take some time, I don't know how long sortPackageMap takes to run), rather than writing them in a sorted format in the first place?

If so, can you propose a way to do this? I'm a bit lost. AutoloadGenerator hasn't been initialized at the point where plugins are loaded, and it doesn't really seem appropriate to use it in the context of the plugin loader anyway, it would introduce an odd and unnecessary coupling. I'm wondering how you think we should sort packages (presumably in PluginManager::loadInstalledPlugins()).

@danepowell

This comment has been minimized.

Copy link
Contributor Author

danepowell commented Apr 6, 2019

@Seldaek actually I think I figured out a way to do this, seems to be okay performance-wise, I'll let you be the judge on architecture: #8085

@emb03

This comment has been minimized.

Copy link

emb03 commented Mar 26, 2020

I am getting this message with Composer version 1.10.1 and BLT 11.3.0.
running blt setup
Could not scan for classes inside "/Users/chris/Sites/devdesktop/itablt2/vendor/drupal/core/lib/Drupal.php" which does not appear to be a file nor a folder Could not scan for classes inside "/Users/chris/Sites/devdesktop/itablt2/vendor/drupal/core/lib/Drupal/Component/Utility/Timer.php" which does not appear to be a file nor a folder Could not scan for classes inside "/Users/chris/Sites/devdesktop/itablt2/vendor/drupal/core/lib/Drupal/Component/Utility/Unicode.php" which does not appear to be a file nor a folder Could not scan for classes inside "/Users/chris/Sites/devdesktop/itablt2/vendor/drupal/core/lib/Drupal/Core/Database/Database.php" which does not appear to be a file nor a folder Could not scan for classes inside "/Users/chris/Sites/devdesktop/itablt2/vendor/drupal/core/lib/Drupal/Core/DrupalKernel.php" which does not appear to be a file nor a folder Could not scan for classes inside "/Users/chris/Sites/devdesktop/itablt2/vendor/drupal/core/lib/Drupal/Core/DrupalKernelInterface.php" which does not appear to be a file nor a folder Could not scan for classes inside "/Users/chris/Sites/devdesktop/itablt2/vendor/drupal/core/lib/Drupal/Core/Site/Settings.php" which does not appear to be a file nor a folder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can鈥檛 perform that action at this time.