Open wongjn opened 4 years ago
I'm also seeing this - on a previous project with composers-installers-extender 1.1, this error didn't happen
I downgraded to 1.1.2, and it's no longer occurring
I tracked down the change that caused this but the composer behavior is super weird and undocumented.
In 2.0 we have this code: https://github.com/oomphinc/composer-installers-extender/blob/8d3fe38a1723e0e91076920c8bb946b1696e28ca/src/Installers/CustomInstaller.php#L16-L18
Where in 1.0 the code looked like this: https://github.com/oomphinc/composer-installers-extender/blob/ca1c4b16b0905c81d1e77e608f36a2eff1a56f56/src/InstallerHelper.php#L7-L15
Now, that ends up interacting with this code in different ways as documented in the comment even though composers behavior seems completely broken. https://github.com/composer/installers/blob/c462a693445b8f6913e95c2ea774d60cf42f64ec/src/Composer/Installers/BaseInstaller.php#L65-L69
The key is these two lines
$packageType = substr($type, strlen($frameworkType) + 1);
if (!isset($locations[$packageType])) {
throw new \InvalidArgumentException(sprintf('Package type "%s" is not supported', $type));
}
return $this->templatePath($locations[$packageType], $availableVars);
Basically what we see is that $type = 'framework'
and $frameworkType = 'framework'
and which mean substr actually fails and $packageType = FALSE
. Then as the comment says, some type conversion happens and FALSE checks index 0, which is actually set.
Next is less important but templatePath returns false because strpos(FALSE, '{')
always fails because false isn't a string and that ends up returning the first argument(false) and that false get returned out of the method triggering a calling method check to use the default.
Now, it doesn't actually make any sense to check the 0th index, relying on undocumented type conversion, relying on passing a boolean into strpos, and the undocumented return types(getInstallPath only claims to return strings which clearly isn't the case). But all this seems to be expected in composer just not documented so I think we just return the old code?
Sorry for the ranting but a single comment along this chain in composer would have saved me several hours of headaches trying to sort this out so the composer team can deal if they end up reading this I think.
PR incoming.
Nice detective work @neclimdul
It seems composer/installers
expects types to be namespaced and hyphen separated. For example drupal-core
, drupal-module
, drupal-theme
. So I'm wondering if this is a documented requirement, and if not perhaps it should be, or perhaps it should be handled differently to allow for this. Or instead of defaulting to library
it should default to generic-library
or similar.
Thanks :)
You're probably right. That definitely makes some sense and then all this is just hacking a value through so handle non-namespaced types can be handled here. https://github.com/oomphinc/composer-installers-extender/blob/8d3fe38a1723e0e91076920c8bb946b1696e28ca/src/Installers/Installer.php#L28
Maybe that's a good suggestion but I think the problem with changing it is its being used in places like chosen and at least with chosen I'm not sure if we're going to be able to change it. https://github.com/harvesthq/chosen/blob/master/composer.json#L30
I agree we probably can't change that. So perhaps we can extend getInstallPath()
instead of getLocations()
? Something like this:
public function getInstallPath(PackageInterface $package, $frameworkType = '')
{
if ($frameworkType == $this->package->getType()) {
// Do something.
}
else {
return parent::getInstallPath($package, $frameworkType);
}
}
That sort of makes sense. My intent was the shortest path to fixing the 2.x branch and getting some composer2 testing so reverting to the previous behavior was my first reaction. I'll see if I can find some time though to continue diving into composer installers and see if the [false] return is interacting with the other calls in anyway or if short circuiting the call in getInstallPath makes more sense.
As a workaround (at least for the chosen library) you can define a custom repository for the chosen library (which is no longer updated anyway) in your project composer.json and set it's package:type to drupal-library. This seems to override the package:type in the chosen library source composer.json.
"repositories": {
"chosen": {
"type": "package",
"package": {
"name": "harvesthq/chosen",
"version": "1.8.7",
"type": "drupal-library",
"dist": {
"url": "https://github.com/harvesthq/chosen/releases/download/v1.8.7/chosen_v1.8.7.zip",
"type": "zip"
}
}
},
},
Then you can require as usual with:
"require": {
"harvesthq/chosen": "^1.8",
},
And define the installer-paths like:
"installer-paths": {
"docroot/libraries/{$name}": [
"type:drupal-library"
],
},
Attempting to use installer-paths options for specific package name (harvesthq/chosen
) or using the vendor prefix (vendor:harvesthq
) seems to get ignored / overwrote by the package:type in the chosen library source composer.json.
I am not sure but I believe the solution is to use https://github.com/mnsami/composer-custom-directory-installer for type library.
@tobiasbaehr that definitely have helped, so thanks! At least in my case, for chosen in drupal.
Basically after requiring composer-custom-directory-installer
and adding to the "installer-paths" section:
"web/libraries/chosen": [
"harvesthq/chosen"
],
chosen is placed in the proper location.
I'm having a similar issue with npm-asset
type packages... but I think it's different from this one. Would love to hear if this is working for anyone else! #32
This appears blocked on the discussion raised at https://github.com/oomphinc/composer-installers-extender/issues/26#issuecomment-705940276 but is addressed in https://github.com/oomphinc/composer-installers-extender/pull/27 which is rather narrow. I think merging that PR (once all outstanding items are resolved re: approach) is the most straightforward approach here.
For what it's worth, adding that PR as a patch to the installer and then re-running affected patches (after adding back library
as an installer type) gets this back to the expected performance.
sorry for the delay. I had to detour through some other problems before getting back to this but doing that audit now.
Cool!
@mstenta So existing tests ended up being really useful here. I started thinking it probably could be pretty simple but tests like InstallerTest::testGetInstallPath
quickly broke and show that you actually need a lot of logic from the parent method in all cases. This I think points back to the current hack in #27 as the only option atm other then kinda forking all the logic from the parent method. That's an option but the code is pretty long and dense so that doesn't seem ideal.
Seems there are no good options here really. Sorry. Let me know what you think.
PS - I found that the plugin tests where failing unrelated to anything I was doing here. I'm not immediately sure how they where previously passing or maybe they weren't but #33 should fix that.
@mstenta sorry for the misstag and congrats on the farmos release! @mstrelan ^^
This PR works for me.
@neclimdul I agree that this approach is the simplest way forward.
For some reason, the PR fixes the problem for me when running composer on PHP 7.4.5, but not when running the same composer in PHP 8.0.3. Literally the only difference is the php interpreter used, and the effect on PHP 8 is still "Package type library is not supported.". I'm currently trying to investigate.
Update: From the earlier investigation by @neclimdul, I think I see what's happening:
substr()
so that it returns ""
(empty string) instead of FALSE
for out-of-bounds indices.""
will not equal FALSE
anymore.(I'm not sure if the second affected array lookups, but whether or not $array[""]
was different from $array[0]
before, it certainly will be now.)
The combination of these two will mean that
$packageType = substr($type, strlen($frameworkType) + 1);
if (!isset($locations[$packageType])) {
throw new \InvalidArgumentException(sprintf('Package type "%s" is not supported', $type));
}
with $type
and $frameworkType
set to the same value will now make $packageType
equal ""
instead of FALSE
, and that $locations[$packageType]
will then be undefined rather than evaluating to $locations[0]
.
I can't begin to understand what composer's intended logic is, but I suspect that we can get the effect we wanted by changing
function getLocations() {
return array( false );
}
to
function getLocations() {
return array(0 => false , "" => false);
}
I hope.
Update: After installing the patch with the above modification, I can add "library" to the installer-types array without causing an error in PHP 8.
Thanks for testing! That's a great find. Also, omg why would they do that.... That's not even very clear from the docs either. It looks very much from the main return docs like it will always return false from my reading. :(
Agreed on understanding composers logic. Its a sea of code without comments but your reading matches my digging and the way I had hacked this in.
btw, free free to provide technical feedback on !27. I'm not sure everyone following this cares about every bug.
Also, this can be really annoying to test since you can't patch it with composer-patches or the like before composer starts running it. This means its impossible to really test a clean install from scratch. For my testing I created https://packagist.org/packages/neclimdul/composer-installers-extender and feel free to use it for your testing as well. But, as noted in the README, I do not plan on maintaining this so don't rely on it. Its only a testing package for this issue that I'll be deleting as soon as a solution for this is merged.
This PR definitely worked for me. +1 for this pull request
Not work for me on composer v2 :(
This works for me:
"extra": {
"installer-types": [
"npm-asset",
"drupal-library"
],
"installer-paths": {
"web/core": [
"type:drupal-core"
],
"web/libraries/jquery.cycle": [
"npm-asset/jquery-cycle"
],
"web/libraries/{$name}": [
"type:drupal-library",
"type:npm-asset"
],
"web/modules/contrib/{$name}": [
"type:drupal-module"
],
"web/profiles/contrib/{$name}": [
"type:drupal-profile"
],
"web/themes/contrib/{$name}": [
"type:drupal-theme"
],
"drush/contrib/{$name}": [
"type:drupal-drush"
]
},
},
It seems to be as simple as drupal-library
or whatever-library
instead of just library
as the installer-type.
@syzygy333 indeed! But you can't control what the packages you want to require have as their types - at least not without overriding the package data as per https://github.com/oomphinc/composer-installers-extender/issues/26#issuecomment-730545360 , which then makes updating/maintaining those packages harder. So in that example of harvesthq/chosen - you can't require that package without doing something awkward, unless this could be fixed in oomphinc/composer-installers-extender .
Is that not what this is doing?
"web/libraries/{$name}": [
"type:drupal-library",
"type:npm-asset"
],
@syzygy333 no - that's configuring where to put each package that you require, of those types. So if you require a package that declares itself to have the drupal-library
type, then it will get placed within that web/libraries
directory. (Great - that bit works as expected, yes đź‘Ť )
But the point here is that if you require an external package which declares itself to be have the type library
(or doesn't explicitly declare its type), oomphinc/composer-installers-extender can't currently support placing the package somewhere in the same way.
I've found a workaround using Drupal Scaffold operations.
I got utterly fed-up of the Chosen library sometimes installing in the right place, and sometimes not, despite having followed the guides above and the work of several other people with the same problems.
As an example, out of the nine D9 sites I maintain - which all use the same composer.json file - Composer on five of the nine sites abjectly refused to put the Chosen library in the correct location, while on the other two sites it worked just fine.
A diff of the composer.jsons between working and non-working sites showed no obvious differences.
So, I'm using Drupal Scaffold as follows:
1: In the root of the repo at the same level as composer.json, I created a build-assets/ directory and added chosen.jquery.js and chosen.jquery.min.js, giving a directory structure of:
./composer.json
./build-assets/chosen.jquery.js
./build-assets/chosen.jquery.min.js
2: In composer.json, in the extra > drupal-scaffold > file-mapping section, add maps which will copy the two Chosen JS files into web/libraries/chosen/ after build:
{
"extra": {
"drupal-scaffold": {
"file-mapping": {
"[web-root]/libraries/chosen/chosen.jquery.js": "build-assets/chosen.jquery.js",
"[web-root]/libraries/chosen/chosen.jquery.min.js": "build-assets/chosen.jquery.min.js"
}
}
}
}
(Obviously the above sections in your composer.json will have other entries in them.)
To test, I ran rm composer.lock; composer install -vvvv
and confirmed that the Chosen files appeared in the correct place, every time.
Hopefully this helps someone else! :)
Alex
Good idea to share a workaround ... I've worked around the problem in a slightly different way, which avoids the need for including the assets in my repo. I define the chosen library as a drupal-library with a repositories
entry, so that composer/installers puts it in the right place (without even needing to use oomphinc/composer-installers-extender
!):
{
...
"repositories": [
{
"type": "package",
"package": {
"name": "harvesthq/chosen",
"version": "v1.8.7",
"type": "drupal-library",
"extra": {
"installer-name": "chosen"
},
"dist": {
"url": "https://github.com/harvesthq/chosen/releases/download/v1.8.7/chosen_v1.8.7.zip",
"type": "zip"
},
"require": {
"composer/installers": "~1.0"
}
},
"only": ["harvesthq/chosen"]
}
],
"require": {
"composer/installers": "^1.9",
"harvesthq/chosen": "^1.8.7"
},
"extra": {
"installer-paths": {
"webroot/libraries/{$name}": [
"type:drupal-library",
"harvesthq/chosen"
]
}
},
"config": {
"allow-plugins": {
"composer/installers": true
}
}
}
This also ends up with webroot/libraries/chosen/chosen.jquery.min.js
đź‘Ť It just means that if I ever want to upgrade the chosen library, I'll need to update the repositories entry. But @alexharriesredactive's suggested solution would also need a manual step (of replacing the downloaded assets).
Yep, except that’s the approach I’m using which all of a sudden only works on two out of nine sites… :-/
On Tue, 5 Apr 2022 at 17:10, James Williams @.***> wrote:
Good idea to share a workaround ... I've worked around the problem in a slightly different way, which avoids the need for including the assets in my repo. I define the chosen library as a drupal-library with a repositories entry, so that composer/installers puts it in the right place (without even needing to use oomphinc/composer-installers-extender !):
{
... "repositories": [ { "type": "package", "package": { "name": "harvesthq/chosen", "version": "v1.8.7", "type": "drupal-library", "extra": { "installer-name": "chosen" }, "dist": { "url": "https://github.com/harvesthq/chosen/releases/download/v1.8.7/chosen_v1.8.7.zip", "type": "zip" }, "require": { "composer/installers": "~1.0" } }, "only": ["harvesthq/chosen"] } ], "require": { "composer/installers": "^1.9", "harvesthq/chosen": "^1.8.7" }, "extra": { "installer-paths": { "webroot/libraries/{$name}": [ "type:drupal-library", "harvesthq/chosen" ] } }, "config": { "allow-plugins": { "composer/installers": true } }
}
This also ends up with webroot/libraries/chosen/chosen.jquery.min.js đź‘Ť It just means that if I ever want to upgrade the chosen library, I'll need to update the repositories entry. But @alexharriesredactive https://github.com/alexharriesredactive's suggested solution would also need a manual step (of replacing the downloaded assets).
— Reply to this email directly, view it on GitHub https://github.com/oomphinc/composer-installers-extender/issues/26#issuecomment-1088943687, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANCE7D3TXRQH4KY5VJWOVEDVDRQWBANCNFSM4RETOF3Q . You are receiving this because you were mentioned.Message ID: @.***>
--
i am not using this in drupal but in wordpress and currently it is a blocker if using php > 8.1 and composer v2. No idea of a workaround i can apply. Does anyone have any idea?
I am not sure but I believe the solution is to use https://github.com/mnsami/composer-custom-directory-installer for type library.
@bizmate We've been using this suggestion but we forked it to mark it as a replacement of this package:
"replace": { "oomphinc/composer-installers-extender": "*" }
@nicolas-girod thank you for the feedback. I ended up using composer 1 again and specifying
installer-types": [
"wordpress-library"
],
as it was suggested previously. I think this is ok as a workaround for now :)
When I run
composer require <package>
with v2.0.0, it errors out:Composer
1.10.13
oomphinc/composer-installers-extender:2.0.0
composer.json
: