Allow Installers to determine installation support based on entire pa…

[davidbarratt]

This PR resolves #4058

[nevvermind]

a) Wouldn't this make $package exposed to change from various installers? Like $package->setId()? Maybe you should use a read-only representation of $package.

b) If I get this right, your code is not BC. Maybe it should be. You're breaking every installer out there. Maybe this grants another interface, installers explicitly declaring that they provide a supports(Package $package) method, otherwise, the old way of passing the type is applied.

What do you think?

[davidbarratt]

@nevvermind,

To your first point, yes it does expose $package to be changed by any installer, but so does every other method in the interface:
https://github.com/composer/composer/blob/master/src/Composer/Installer/InstallerInterface.php

This PR would just allow an installer to modify any package (even ones it technically doesn't support). However, you could do the same thing by making this method always return true. That would mean the entire $package object would be passed into all the other methods and the installer could modify any package. I think you're making a good point, but that's outside of this issue.

To your second point, if I create a new interface and installers can implement InstallerInterface or InstallerPackageInterface (or whatever you think it should be called), doesn't that make it ambiguous? or would the InstallerPackageInterface only have a single method, something like supportsPackage and if installers want to they can implement both interfaces?

[Seldaek]

After discussing this on IRC with @derhasi, I still don't really see why you'd need this in the first place, because it sounds to me like the type ought to be enough for all use cases I can think of, and nobody can explain me why they need more (I mean why you would need to decide on name + type, I understand that that's what you are trying to do, but not the root reason).

One way around this that I see however as a sort of hack from the installer itself, would be:

• store the Composer\Composer instance the plugin gets in the constructor.
• when supports() gets called, always return true for types you're interested in.
• when install/update/... are called, if the package you get isn't one that you are interested in, run this to send that call back to the original installer:

$im = $this->composer->getInstallationManager();
// remove the plugin, which busts the type-to-installer cache
$im->removeInstaller($this);
// fetch the actual installer that should have been used
$installer = $im->getInstaller($package->getType());
// re-run the install/update/...
$installer->install($repo, $package);
// re-add the plugin
$im->addInstaller($this);

This isn't really pretty, but at least it lets you do it without changing composer. The alternative is to convince me what you are trying to do is a sane idea and then fix this patch somehow so it's BC.

[derhasi]

@Seldaek, we are already working around the given constraints with forcing the custom-config to contain the definition for the type. (see davidbarratt/custom-installer#6)

From my perspective that would be fine. But maybe @davidbarratt can add a valid use case that would justify an API change.

• IRC log

[davidbarratt]

@nevvermind & @Seldaek

This PR does a couple things, it standardizes InstallerPackageInterface so that every method is passed the entire object rather than just the type.

Because of this change, developers can make more interesting Installers that are not necessarily tied to a specific type.

For instance composer/installers was forced to add support for a drupal-core type in composer/installers#65, even though, as far as I know, it's only been implemented a single time:
https://github.com/drupal/drupal/blob/8.0.x/core/composer.json#L4
Because of the limitation of the api, this is really the only way it could have been done (at least in composer/installers)

There's another custom installer out there composer-custom-directory-installer that allows a package to be installed in any path, but as @derhasi has pointed out, it only works on type library (again, because of the limitation of the API)

This brings us to davidbarratt/custom-installer#6, I created Composer Custom Type Installer around the limitations of the API (that is why it is strictly type based at the moment and I really didn't have any plans to change that), but @derhasi submitted that PR to give Composer Custom Type Installer the same syntax as composer/installers. However, composer/installers only works with the pre-defined types, if you use any other types, it wont work (again, this is mostly because of the API). Since there are no predefined types in Composer Custom Type Installer, the only thing we can do is just takeover the entire install process for every type, this forces the user into some strange work-arounds.

I guess I just don't understand why the API is strictly just type based when installers should be able to use whatever they want to insert a package into different places.

Another good example of this is Drupal 7 modules vs. Drupal 8 modules. They need to be installed in two completely different places, but they have the exact same type (see composer/installers#44). Wouldn't it be nice if you could look at the module's version number (or the module's drupal/core dependency version) and determine the install location based on that? otherwise we'd have to change the type for every module and it would be a different type for 7 vs. 8 which is silly since the only reason we'd be doing that is for this API limitation.

I've seen this problem mentioned in regards to other libraries that composer/installers handles, not just Drupal. See composer/installers#217 . Honestly I can only see this becoming more of a problem not less of one.

As it was pointed out on IRC, this does break the "caching" a bit. I changed the cache to be by package name (which might be useless). I'm actually not even sure how much the cache is saving. I suppose when it's per-type it's saving some. However, it's pretty quick/easy to determine the packages an installer supports and a lot of installers just return true and takeover all installations as @Seldaek described. If we do this, we are (and anyone else) is breaking the cache anyways, which in some ways makes it even less useful since it's an unreliable cache. I think it's ok if installers cache the data themselves. Some installers will be able to cache on a type basis, others will use other methods of caching. At the very least we can cache by unique package name so if the method is called again for whatever reason, it wont have to process it for that package.

As far as preventing a BC break, I'm not sure what the best way to do that, I suppose we could have a new interface and Composer can search for either implementation?

[nevvermind]

@davidbarratt - To be honest, I rather like the idea of letting the installers decide at runtime if they can handle a package or not.

But.

People will always push their features with the force of necessity, like it's clearly a good feature. What can go wrong, right? Try putting yourself in Composer's dev team for a minute:

• "installers should be able to use whatever they want to insert a package into different places" - that's the worst possible phrasing you could use when talking to an API developer. It's like painting your feature with danger-red paint! 🔴 API devs become reactionary on their very first release.
• Keeping the plugins from having too much power is not always a bad thing. It's the API being on a need-to-know only basis.
• You're not "just making the installers more flexible", you're actually changing a subtle behaviour in the codebase (from what I've seen by browsing it anyway): the code is assuming one handler per type and is acting on behalf of that assumption. What you're doing is actually making an installer handle one or more types. That's an internal paradigm shift. Not necessarily a bad one, but a big change nonetheless.
• I've never seen them before, but I absolutely have no idea why those installers declare types based on only their names. They actually assume that the install process is the same on every version?! Why not have drupal8- and drupal7- types? Is this Composer's fault or the convention's?
• Re Drupal/Wordpress etc. Their problem with Composer is deployment. While Composer timidly tresspasses on a couple of occasions (Yeah, I'm looking at you, create-project), don't forget that this is foremost a dependency management tool and the tendency is in that direction. Everything deploy-related is taken as less important (I'm assuming, ofc. I couldn't know their agenda). Why would Composer care for it anyway?
• Keep it simple, as this is the most complicated thing to achieve.
• Consider that some communities will push the complexity higher because they're asking the tool to change, instead of their own processes, deployment etc. Basically, you'll have more chances of your work being merged by not talking about frameworks and keeping things generic.

Wow, I digressed quite a bit. I know it's more about general theory than your work, but hopefully you can now relate more easily with "the other side". 😁

Regarding your actual PR, I honestly don't know what to say. I'm not 100% convinced. More like 90%. But I do understand that some of you might need a more finer control. I'm working with a custom Magento installer myself. And we can manage pretty well. And if we can manage it with Magento, I think Drupal/Wordpress or others can, too.

[Seldaek]

And if we can manage it with Magento, I think Drupal/Wordpress or others can, too.

That's the thing.. Yes drupal8 could use drupal8-module as type, and that's that, or they could use the chance to go one step further and use Puli for example to find which modules are installed and auto-register them, if the symfony approach of having a user-managed list of enabled plugins isn't deemed good enough.

There are many solutions out there, none of them requiring custom installers.. They are already a bad idea as it is, creating edge cases and non-standard behavior in projects, and I really don't think that giving them more power is a great idea. They should be a bit painful to use so that people notice and consider migrating away, because having code that needs dependencies to be in directory X or Y made sense 10years ago but nowadays with autoloading everywhere and composer in the middle it is just useless historical cruft.

[davidbarratt]

@nevvermind & @Seldaek

I come from the Drupal API world where the API allows you to pretty much modify anything, and honestly could be completely destructive (i.e. you could make a module easily that stole user data, wiped out the database, or injected ads into a sites page, etc. etc.). Of course this rarely happens, I've yet to see a Drupal module that was so bad that it was unsafe to use.

I completely disagree that giving developers more flexibility and more power is a recipe for disaster. You simply do not see that. (ok, fine, there are some malicious WordPress plugins out there, but that's not really the API's fault). In fact, I would argue the opposite, the more power you give them, the better their solutions (see drupal modules vs. wordpress plugins).

As far as trying to move these big projects (like WordPress, Drupal, Magento, etc.) in the right direction (by putting everything in vendor) I think is a good idea, and I agree that putting dependencies all over the place is an archaic solution, but it's kind of what we have at the moment and I think it would be a huge BC break for these projects to change. I suppose if the goal is to force everyone to move to vendor then you should reject this PR (actually you should reject all custom installers by the same logic, but whatever). By sticking with that logic, it makes it difficult for users of Composer (like me and @derhasi) to use Composer in a meaningful way in our projects. This really isn't a question of DX, it's a UX problem. If you want to make it easier for users who use these big libraries in their projects to transition to using Composer for everything, then I would consider opening up the API a little and allowing developers to use Composer in a more meaningful way. I think it should still be documented that the best user experience is for libraries to be installed in vendor, but for these big projects... that's a long time from now. :/

[nevvermind]

@davidbarratt - Maybe I'm a little pessimist when it comes to 3rd parties.

Don't get me wrong, I'm not against flexibility, but against unnecessary one (which admitedlly, can be somewhat of a subjective matter).

Case in point, I'm actually in the process of trying to clear the way for #3377 to get into the codebase. That PR means stuff like composer drupal8.install --theme="Zen" or composer wordpress.deploy --everything which is 😎 . Would custom commands help with your problem?

[davidbarratt]

@nevvermind

I mean if you can dictate where a package is installed with a script then sure! but I'm not sure how that would work. :/

[alcohol]

I'll just pitch in my 2 cents, which have always been the same when it comes to issues like these. Composer is not a swiss army knife. It's not a tool for many jobs. It's a tool for one job. And it does that job pretty good. Everything extra that you get with it is merely a convenience but should not be relied upon. It should also not be an excuse to add more conveniences. Every job has the right tool. Your scenario can be solved with many other tools. There is no need to make composer accommodate this as well.

The day and age that configuration is done based on paths and directories, is long behind us. Stop using such deprecated and inconvenient methods for managing configuration.

[davidbarratt]

sigh follow to logical conclusion... there shouldn't be any Installer Plugins.

[alcohol]

That would have my preference 👍 .

[timmillwood]

I'm really struggling to see the downside of this PR. It makes Composer much more flexible and allows the ecosystem to flow and adapt much easier.

There are a lot of systems and platforms (Heroku, Redhat's Openshift, etc) that are not embracing composer during deployment, and if we can now hook into that it makes CI so much easier.

[Seldaek]

@timmillwood @davidbarratt can we get an update here? Is this still relevant? I'm still not so convinced but now that you delved deeper in drupal-composer integration maybe you have a clearer picture of why you'd really need it, or whether you managed to work around and we can just close it?

[davidbarratt]

@Seldaek,

I think it still would be really helpful for plugins to determine the install path based on the entire object. All other plugins get the opportunity of interacting with the whole object; the only reason install plugins don't is because of an ideological reason that doesn't even make any sense.

This PR isn't about Drupal, it's about all installation plugins.

I still think this PR is the best direction for install plugins and Composer as a whole; but if you disagree then that's fine.

[Seldaek]

It's fine to call things silly and senseless, but it's better to do it with an actual opposing proposal that makes sense. I still am not convinced that there is a valid use case for this that can't be served well enough with types.

I'll give @timmillwood a chance to answer before closing though.

[davidbarratt]

By closing this PR you're saying that Composer's APIs are inconsistent by design. Are we OK with that?

[Seldaek]

Please stop with the threats, it's absurd.

[davidbarratt]

It's not a threat at all; I just don't understand the logic.

[Seldaek]

I am not saying that the APIs are inconsistent by design, I am saying that they are what they are due to a mix of historical and meaningful reasons. Regardless of whether you agree with those reasons or not, it's the state we are in. Changing things has a cost, so if I don't see the benefit I don't see why I should inflict that cost on everyone.

[davidbarratt]

that's legit. I don't really know what the cost would be, so I don't really have a say in that.

[Seldaek]

Breaking all existing custom installers is the main issue. The other point is that I still think that restricting what people can do does sometimes bring good discussions to the table and therefore better solutions than if people can just hack it any way they want. That's more subjective I suppose.

[timmillwood]

I think that the current implementation with just a $type string works ok, and works for 90% of the use cases.

Although this PR is good for 2 reasons:

1. It uses PackageInterface and therefore enforces better structure.
2. It allows more flexibility, which is never a bad thing.

However I think we need to be 100% sure this doesn't break existing custom installers.

[Seldaek]

@timmillwood those arguments are valid on a theoretical level but it still doesn't give me an actual use case where it's needed. I'm not keen on breaking things for purity's sake, so let's close this for now.