Use Puli discovery

[sagikazarmark]

I have been looking at puli for the last few weeks. I think @joelwurtz also spent some time with it, at least during PHP Forum with @webmozart.

I think it is quite awesome and we could easily use it not just in the discovery package, but in the whole project. Since we have nice contracts, we already have out binding types: HttpClient, HttpAsyncClient, *Factory. In each of our packages we can provide a puli.json with the actual bindings (like Guzzle6HttpAdapter is bind to HttpClient and HttpAsyncClient)

Downside of this solution is that every resource must be registered in the repository (?) and conditional auto-discovery (and caching) during runtime won't work. However, this is not necessarily a downside, but probably makes flexibility a bit harder.

While this could make our static discoveries obsolete, I would still keep them. Although Puli does not recommend it to use it in global context or instantiate it more than once, I think we should use them inside our discovery layer. The workflow would be the following:

If someone wants to resolve something from the discovery, and it is the first call to any discoveries, we instantiate the Puli factory and store it in the ClassDiscovery object. After that, inside the specific discovery (like HttpClientDiscovery) we loop through our internal list of clients and append them to Puli ONCE. We set an initialized bit or something. This replaces our internal cache mechanism. After that, we resolve the first match of the specific binding and return it.

Possible pitfalls:

• Performance: Puli recommends NOT to this global context thing for performance (and other) reasons. However our discovery thing's aim is zero config, which itself is not recommended. Configuration, DI should be used, but if you need it as is, you can use it. Not sure if we need to care about this apart from placing a big fat warning somewhere.
• Priority: not sure about Puli's priority handling. FIFO? LIFO? Does it really matter? IMO when we use a discovery, we just want something fast, should we really care about priorities? Currently it is possible to add resources to discoveries and they are returned FIFO.
• Puli required: with this we would add puli as a dependency. Not sure if it is really a downside, as puli is quite awesome. Putting a puli.json file in all of the related repos should really not be a problem.

Reference:
https://speakerdeck.com/webmozart/puli-phps-next-package-revolution?slide=102

[dbu]

oh, i was not aware that puli can provide objects - i thought its about resource files and such things. but then, why not ditch discovery completely and document how to use puli for autodiscovery? the only thing we would need to provide is a puli.json? the current discoveries are not really complicated, and they are a last resort thing, i would like to avoid overcomplicating them. we might also want to look at how laravel is doing things - the discovery looks a lot like what laravel erroneously calls "facade".

[sagikazarmark]

Well, I have one reason against completely dropping discovery: right now we accept for example a nessage factory in the constructor. If that's not found, we use a static call to fallback. Puli discovery is not static. It is not recommended to instantiate the main object multiple times.

So how do we access it? Inject it in the constructor as well?

i would like to avoid overcomplicating them

I tend to agree, but the API wouldn't change, just the internal implementation. While it adds complexity, from a user point of view, there is no extra logic.

[dbu]

how much more flexibility does it bring? how much complexity from our own code can we drop in exchange for the added complexity of using a third party tool? (this is real questions, if we can answer them positive, i am +1 to do this)

[sagikazarmark]

how much more flexibility does it bring?

In general, the only problematic things are the message factories. In their case, it is not enough to add them to the discovery service, we need to check if their underlying implementation is installed. Otherwise we can't use them, even if they are in the discovery. That's why it would be nice to have message factories inside implementations.

In every other case we can just replace our discovery logic with puli, as the information can come from the puli.json of the package. If the package is installed, the http adapter for example is available from the discovery service (autodiscovery).

how much complexity from our own code can we drop in exchange for the added complexity of using a third party tool

As mentioned above: all of the complexity can be dropped except in case of factories. Maybe we could find a way and add a custom binding type, which can include the condition. In this case we can just check if the condition for a binding is fulfilled, if not, move on to the next binding.

Apart from removing complexity, we add a powerful feature as well. For example in HttplugBundle: you can rely on the puli discovery, instead of our global one as a fallback. You can inject the discovery into whatever class where you resolve the fallback.

[dbu]

sounds good then!

[webmozart]

In general, the only problematic things are the message factories. In their case, it is not enough to add them to the discovery service, we need to check if their underlying implementation is installed. Otherwise we can't use them, even if they are in the discovery.

What exactly do you mean by that? Is there any way I can help?

[sagikazarmark]

Hey @webmozart. It's nice to see you here.

I'll try to describe the problem, maybe you can help us with a solution.

Currently we use two PSR-7 HTTP Message implementations: Guzzle and Diactoros. We also provide Message Factories for them.

Until now we had Message Factory implementations in the same repo as our discovery classes which made it simple to use any of our factories. The only thing we had to check is the available message implementation: if guzzle implementation was available, we returned the guzzle message factory from the discover, the same with diactoros.

Now we changed the plan a bit: we would like to move message factories to another package, because they doesn't really belong there. We can add a puli.json to that package with a binding to the appropriate message factory interface, BUT we still have to check if the matching message implementation is installed. If a message implementation is not installed for the message factory, we can't use it.

With other words: we need a double-discovery: first need to check the available message factory, then check if message implementation is available. In an ideal case, factories should live together with their implementations, but unfortunately, this is not possible in our case.

So we need a solution to create custom bindings for example with extra properties/attributes? So when we consume a binding from the discovery, we can decide whether we can actually use it (based on these extra attributes) before returning it or moving to the next binding.

[webmozart]

Maybe that's a stupid question: why don't you ship the message factories with the same packages as the corresponding messages?

[sagikazarmark]

Because we don't actually maintain the message implementations. I submitted a PR to Guzzle with message factories, but it was rejected.

[sagikazarmark]

As I can see binding types can be defined with parameters. Can we use that to store for example a class name which we can check when we get the binding from the discovery?

[sagikazarmark]

Started to play with Puli: https://github.com/php-http/puli-test

[webmozart]

Ah that makes sense. Yes, you can store arbitrary scalar/null values in binding parameters.

[sagikazarmark]

@webmozart do I have to define the parameters in binding types?

Actually Guzzle and Diactoros factories are kind of special ones, as they are not packaged together with their message implementations. But as you pointed, normally they should.

That's why I added an implementation parameter only in the binding, but not the binding type: https://github.com/php-http/utils/blob/puli/puli.json

The problem is that bindings are not found this way. As soon as I remove those parameters, they are found.

Is this a bug or an intended feature?

[webmozart]

Yes, you need to define parameters upfront. Think of binding types like of interfaces. They define a contract that needs to be fulfilled by the bindings.

The problem is that bindings are not found this way.

That's strange, they should. Can you provide more details?

[sagikazarmark]

Yes, you need to define parameters upfront. Think of binding types like of interfaces.

Yeah, I guessed. Actually I don't want to make it a requirement. I want to make it optional only for these two factories, so I don't want to make them part of the "interface".

That's strange, they should. Can you provide more details?

https://github.com/php-http/puli-test

Install composer dependencies and run php factory.php. Factories should be listed there based on these configurations:

https://github.com/php-http/message-factory/blob/puli/puli.json
https://github.com/php-http/utils/blob/puli/puli.json

When I remove the parameters from the installed utils package in the vendor directory, the factories are correctly found. So it seems that when the "interface" differs from the "implementation", the binding is not correctly found.