Do not pull unneeded dependencies

[gjuric]

Hi, I am asking you to reconsider making psr/cache and psr/log hard dependencies of this project. The change done in #36 does not bring any benefits to this package.

If somebody wants to use one of the plugins that depend on the psr/log and/or psr/cache interface they will have to install a concrete implementation of those interfaces. When they do that, those packages will pull in the required interfaces.

If we leave it as it is, we are just adding one more package to install that is not always needed.

[Nyholm]

If somebody wants to use one of the plugins that depend on the psr/log and/or psr/cache interface they will have to install a concrete implementation of those interfaces. When they do that, those packages will pull in the required interfaces.

That is not correct. The psr/log and psr/cache do only contain interfaces. They will not require any concrete implementations.

The virtual packages psr/log-implementation and psr/cache-implementation however requires you to have an concrete implementation. But we do not have a dependency on those.

[gjuric]

Sorry, I think maybe I was not clear enough.

I wanted to say that if you want to use a caching plugin, you must have a caching implementation. The same goes for if you want to use the logging plugin, you need a logging implementation. I do understand that you are just type hinting via an interface and that this does not mean I need to have an implementation if I am not using those plugins, but the interfaces get pulled in nevertheless.

We can see that the caching and logging libraries depend on this interfaces, like https://github.com/tedious/Stash/blob/master/composer.json#L29 and https://github.com/Seldaek/monolog/blob/master/composer.json#L17.

So we have 2 use cases:

1.) You want to use caching or logging plugin, you install a psr/log or psr/cache compatible implementations and they bring in the interfaces as their dependencies.

2.) You do not want to use either of those, the interfaces do not get pulled in.

[Nyholm]

Hm, I understand your point that those interfaces will be pulled if you use an implementation.

Since we are using a class/interface from an other package (https://github.com/php-http/plugins/blob/master/src/CachePlugin.php#L43) we should have that package as our requirements. Our goal, like most packages, is to have as few dependencies as possible. To have a dependency on a package with interfaces are considered very lightweight.

---

The issue you really are concerned about is that we are violating the common reuse principle. I think you mean that this package is too big and you only use a fraction of the classes and only some (or none) of its dependencies. To tackle that problem you should split this package into many smaller packages. But that will make it more difficult to install and maintain.

[gjuric]

Well, the thing is that it is not really a requirement for the package, only for a small part of it, and if that part is going to get used the implementations will bring in the interfaces.

I think maybe the best solution would be to add psr/log-implementation and psr/cache-implementation to the list of suggested packages.

Although, I do see an issue with my reasoning if we get new versions of psr/log and psr/cache interfaces, but I am not sure if that is feasible. Even if it happened, the respected plugins could be extracted to a separate package.

I agree it doesn't make sense to split every plugin into a separate repository, but what would maybe make sense is to extract the Plugin interface into a separate repository. That would allow anybody to use just what they need.

[sagikazarmark]

Hm, I think there are many reasons pro and kontra. It might make sense to look up the commit which actually added these as dependencies to understood the reasoning which I don't remember.

One thing can be versioning, which is actually a very serious one. Even if not having a high chance, any BC incompatible change in those interfaces will end up in incompatibility with a long list of versions of this library.

what would maybe make sense is to extract the Plugin interface into a separate repository.

This is an interesting idea and I think we had a discussion about it in the past. At that time we decided to have everything in one repo. Since most the plugins does not have external dependencies (apart from the interfaces packages of course and our message package), we thought the extre 200-300 LOC would not cause any trouble in one repository.

I would wait for some other opinions, because I think this is not an easy decision. The more input comes, the more easier it becomes.

[Nyholm]

Sorry for being very binary here :)

Well, the thing is that it is not really a requirement for the package, only for a small part of it

That means that it is a requirement for the package. Yes, it is just a small part but size does not matter...

[dbu]

i tend to agree with gorans reasoning. you can't use cache / log without an implementation, so the user will need to install an implemenation anyways and the implementation in turn depends on the psr interfaces. for the potential of BC breaking changes, we could add a `conflicts` section to prevent installing of PSR versions that we don't know at this point. a conflict with log >= 2.0.0 and cache >= 2.0.0 should do that, right? then we have the same constraint as we now have, but without automatically pulling in interfaces that we don't use alone anyways.

[Nyholm]

i tend to agree with gorans reasoning. you can't use cache / log without an implementation, so the user will need to install an implemenation anyways and the implementation in turn depends on the psr interfaces.

Sure that is correct but not relevant. If you are using a class/interface/trait from another package you MUST require that package in composer.json. This would be a no-brainer in a compiled language because you would get a compile error due missing classes.

Im happy to discuss options for package design and architecture to be able to reduce dependencies, but just to remove entries in composer.json is not an option.

[gjuric]

The issue you really are concerned about is that we are violating the common reuse principle. I think you mean that this package is too big and you only use a fraction of the classes and only some (or none) of its dependencies. To tackle that problem you should split this package into many smaller packages. But that will make it more difficult to install and maintain.

OK, so we agree that this is the real issue. But if the only reason you dumped all of the plugins into one repository is for maintenance issues, than it gets harder to argue the requirements part as "binary".

Also, I do not see the "compiled language" as a valid argument since the reason the compiled language would complain is that it would actually be able to do a static analysis, and that is the real reason why it would need to see the interface for itself.

I think that @dbu 's solution to lock the interface version via the conflict section ticks all the boxes.

[sagikazarmark]

Well, Looking from another perspective: this is not different from httplug. In case of httplug we suggest requireing client-implementation, not the interface itself. And if someone is really mad, he/she could create a package providing this package, while not requiring nor implementing our interface.

Another difference is that in our case client-implementation would be in the suggest part of composer, not require (since it is not a requirement).

Nevertheless: @gjuric can you please rebase your branch?

[dbu]

the reason java or such would need the psr is so they can compile the plugin without a concrete implementation available. this is not an issue for us here. as long as you are not using the CachePlugin, you don't need a caching implementation. this is why we do not depend on the caching implementation. so we already have the compromise of allowing to install the plugins without a caching implementation. not having the psr interfaces for caching has no additional drawback (apart from the weird trolling idea mark explains, but i think we all agree this is not a relevant situation).

[sagikazarmark]

Agree, this has no downsides nor BC break IMO. If we ever feel the need to put it back, we can do it.

[gjuric]

I have rebased the branch. Should I add the conflict part as well, what about the recommend?

If so, @dbu can you confirm that this would be an OK syntax?

    "conflict": {
        "psr/log": ">= 2.0.0",
        "psr/cache": ">= 2.0.0"
    },

[Nyholm]

I've made it very clear that I disagree with this change. I do not see any good arguments to remove it excepts that it "feels wrong", "you do not need it" and "if you need it, you will have it anyways". The case of this PR is that you do not want to pull down four very common interfaces to your vendor directory.

Like I said, I think this is a step away from good package design. I do not think we can get out more of discussing this in length ATM. I cave for the majority, lets merge this and we can bring this discussion back at a later time.

---

If so, @dbu can you confirm that this would be an OK syntax?

This would in theory be better but the PSR's will never update the major version. See PSR-0 and PSR-4. I do not think a conflict is required.

[dbu]

yep that looks correct. please add that.
the recommend already is there, we recommend log and cache implementations (which is enough, and you do need the implementation and not only the psr) so no need to adjust that i think.

[gjuric]

All done, I didn't notice it was already in the suggest section.

[joelwurtz]

I'am +0 with this, don't think of any benefits despite having less packages (but psr packages are very small and add no overhead). Both way are fine too me.

[sagikazarmark]

@Nyholm although I do agree with your point/example about compiled languages, in our case the fact of being a collection package with optional dependencies is somewhat an argument on the other side. The ultimate solution would be separated plugins with clean dependencies, but we decided not to do that.

Although in case of log and cache plugins we might make an exception, but that would also require to separate the plugin interface as well. Not sure if we want that (hashtag dependency hell).