-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
BC break in 2.2.8 #10618
Comments
Can reproduce, thanks, investigating. |
Seems due to d679532 but I don't yet understand why that would cause such a messy outcome. |
OK so the problem is this:
Now due to the commit above, This changes the sorted list of plugins to be loaded from:
to:
So acquia/blt now loads before composer/installers, and so Composer isn't yet aware of the custom install paths at the point where it loads BLT's dependencies. This is ultimately a problem of drupal/core not requiring composer/installers, so that even tho blt requires drupal/core, Composer cannot figure out that composer/installers is a dependency and thus should be loaded first. I am not sure what to do here. Ideally it would be resolved by adding the appropriate require in drupal/core (and other packages that do depend on custom types from composer/installers to be installed in the right location). Another workaround would be for blt itself to require composer/installers, but I would only do that if blt is only ever used within projects that have composer/installers installed anyway (which I guess is the case but I am not 100% sure?). The question for me is.. can this be resolved on your end in a way that does not cause lots of pain for the community or not? If not then I guess I can rollback the change, or add some custom hack for this in Composer to keep the existing ecosystem working, but I'd obviously rather not do that if I can avoid it. # |
Thank you for the thorough investigation and response. I agree, this should be solved at BLT's (or it's dependencies' end) if we possibly can! Unfortunately this issue broke our CI which always runs the latest composer. We've locked to 2.2.7 which will be a good enough solution for now. An upstream fix may take some time and will require everyone to update, but given the workaround is fairly easy and the problem is the actual packages, I am also inclined to not try and build in a custom hack for this. However, is this actually a broader issue affecting composer installers? My (possibly incorrect) understanding is that at this point, an install has already happened and composer installers has already changed the install location. From that point on, the new locations need to have the top most priority, regardless of whether something depends on composer installers? Whilst this issue is related to Drupal's requirement of a different install location, I expect this could break any root project |
IIRC you cannot just choose that, you can only override paths for package types supported by composer/installers, and those packages generally probably should be requiring the plugin. Some other plugins may allow you to do crazy things tho, and could break Composer in various ways for sure :) |
i think the reason BLT doesn't currently require composer installers is that the project scaffold we assume you'll use (e.g. https://github.com/acquia/drupal-recommended-project/blob/master/composer.json) typically does. So... adding composer installers to BLT seems like a bit of a hacky solution, since composer installers should be coming into the project already due to the scaffolding in use. |
having said that, we could add composer installers to blt, i guess just worry that if it's truly alphabetical, are we kicking the can down the road and will run into this in the future if an "a" named package has a similar issue? |
So my understanding is that it is alphabetical when not determined by a dependency. So package I think the 'can down the road' question is whether a dependency can be legitimately moved if the dependency itself doesn't depend on Package A: Doesn't have a requirement to be in a particular directory. In the above scenario, Package B breaks unless either it or Package A adds |
It's not alphabetical, it's first sorted by dependency order, which is why adding the dependency would solve it. Alphabetical is only for packages with the same dependency "weight". Anyway, I agree BLT should not have to require it. drupal/core definitely should though if it is expecting to be installed in a custom path always. But anyway I think we should probably add some way for plugins like composer/installers to identify themselves as modifying the install path, and then those should be prioritized because if they are not loaded things can go very wrong. That would also fix the issue here. I'll get a 2.2.9 out later today or tomorrow latest with this. |
Apologies for the cross post above! I think your suggestion for 2.2.9 is actually the most robust solution! |
I've removed the configuration from BLT in the latest release: |
Alright, https://github.com/composer/composer/releases/tag/2.2.9 is out - hopefully this fixes it for good! |
Looks grand and works a charm. Thank you for investigating and fixing so quickly! |
Reproducible with Drupal's composer template: https://github.com/drupal/recommended-project/blob/9.1.x/composer.json with the addition of acquia BLT:
When I run any composer command (including diagnose) I get the following:
I believe this is probably the fault of acquia/blt having:
However, this is only an issue in 2.2.8 released this morning. Rolling back to 2.2.7 fixes the issue. Something has changed in the way it works out the eventual install path / autoload. From the release notes, I wonder if it is related to the change to alphabetical ordering.
The text was updated successfully, but these errors were encountered: