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

Package type "library" is not supported #26

Open
wongjn opened this issue Sep 10, 2020 · 31 comments · Fixed by TopFloorTech/composer-installers-extender#1 · May be fixed by #27
Open

Package type "library" is not supported #26

wongjn opened this issue Sep 10, 2020 · 31 comments · Fixed by TopFloorTech/composer-installers-extender#1 · May be fixed by #27
Labels

Comments

@wongjn
Copy link

wongjn commented Sep 10, 2020

When I run composer require <package> with v2.0.0, it errors out:

$ composer require monolog/monolog
Using version ^2.1 for monolog/monolog
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 2 installs, 0 updates, 0 removals

Installation failed, reverting ./composer.json to its original content.

                                           
  [InvalidArgumentException]               
  Package type "library" is not supported  

Composer 1.10.13
oomphinc/composer-installers-extender:2.0.0

composer.json:

{
    "require": {
        "oomphinc/composer-installers-extender": "^2.0"
    },
    "extra": {
        "installer-types": ["library"]
    }
}
@malcomio
Copy link

malcomio commented Oct 7, 2020

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

@neclimdul
Copy link

I tracked down the change that caused this but the composer behavior is super weird and undocumented.

In 2.0 we have this code:

class CustomInstaller extends BaseInstaller
{
}

Where in 1.0 the code looked like this:

class InstallerHelper extends BaseInstaller {
function getLocations() {
// it will be looking for a key of FALSE, which evaluates to 0, i.e. the first element
// that element value being false signals the installer to use the default path
return array( false );
}
}

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.

neclimdul added a commit to neclimdul/composer-installers-extender that referenced this issue Oct 8, 2020
@neclimdul neclimdul linked a pull request Oct 8, 2020 that will close this issue
@mstrelan
Copy link

mstrelan commented Oct 9, 2020

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.

@neclimdul
Copy link

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.

return $path ?: LibraryInstaller::getInstallPath($package);

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

@mstrelan
Copy link

mstrelan commented Oct 9, 2020

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);
    }
}

@neclimdul
Copy link

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.

@timwood
Copy link

timwood commented Nov 19, 2020

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.

raffaelj added a commit to raffaelj/cpmp-lib-skeleton that referenced this issue Dec 14, 2020
To prevent

```
[InvalidArgumentException]
Package type "library" is not supported
```

after running `composer update --no-dev --ignore-platform-reqs`

see also: oomphinc/composer-installers-extender#26
silverham added a commit to silverham/personal-website-new that referenced this issue Jan 4, 2021
@tobiasbaehr
Copy link

I am not sure but I believe the solution is to use https://github.com/mnsami/composer-custom-directory-installer for type library.

@hanoii
Copy link

hanoii commented Feb 22, 2021

@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.

@mstenta
Copy link

mstenta commented Mar 11, 2021

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

hron84 added a commit to hron84/blog.hron.me that referenced this issue Mar 22, 2021
The drupal/fontawesome package does not require the Font Awesome library
on its own, so we have to hack it around with custom install path to
enforce the package into the Drupal libraries folder.

oomphinc/composer-installers-extender#26
https://www.drupal.org/project/fontawesome/issues/2839928
@bradjones1
Copy link

This appears blocked on the discussion raised at #26 (comment) but is addressed in #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.

@neclimdul
Copy link

sorry for the delay. I had to detour through some other problems before getting back to this but doing that audit now.

@neclimdul
Copy link

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.

@neclimdul
Copy link

@mstenta sorry for the misstag and congrats on the farmos release!
@mstrelan ^^

neclimdul added a commit to neclimdul/composer-installers-extender that referenced this issue Apr 5, 2021
@nterbogt
Copy link

nterbogt commented Apr 6, 2021

This PR works for me.

@mstrelan
Copy link

mstrelan commented Apr 6, 2021

@neclimdul I agree that this approach is the simplest way forward.

@cburschka
Copy link

cburschka commented Apr 14, 2021

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:

  • PHP 8 has changed substr() so that it returns "" (empty string) instead of FALSE for out-of-bounds indices.
  • PHP 8 has also changed non-strict comparison so that "" 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.

@neclimdul
Copy link

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.

@neclimdul
Copy link

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.

@jonnyeom
Copy link

This PR definitely worked for me.
+1 for this pull request

@dantranv
Copy link

dantranv commented Jun 21, 2021

Not work for me on composer v2 :(

image

@syzygy333
Copy link
Contributor

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.

@anotherjames
Copy link

@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 #26 (comment) , 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 .

@syzygy333
Copy link
Contributor

Is that not what this is doing?

"web/libraries/{$name}": [
            "type:drupal-library",
            "type:npm-asset"
        ],

@anotherjames
Copy link

@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.

@alexharriesredactive
Copy link

alexharriesredactive commented Apr 5, 2022

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

@anotherjames
Copy link

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).

@alexharriesredactive
Copy link

alexharriesredactive commented Apr 5, 2022 via email

@bizmate
Copy link

bizmate commented Jun 1, 2023

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?

@nicolas-girod
Copy link

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": "*" }

@bizmate
Copy link

bizmate commented Jun 1, 2023

@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 :)

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