Don't recommend pimple-interop

[akrabat]

mouf/pimple-interop is based on Pimple 1. The Pimple project is currently on version 3.0.2. We probably don't want to be recommending Pimple 1.

[weierophinney]

It doesn't look like v3 has container-interop support; is there a project that provides that?

(Also, completely missed that pimple-interop was using v1!!!!)

[harikt]

Why not send a PR to make it compatible then ?

[geerteltink]

First of all, I actually had a class in the expressive installer that made it work on a very basic level. I don't know the current status of the PSR, all I know is it needs a get and has function. This is easily done: https://xtreamwayz.github.io/blog/pimple-3-container-interop/. But then again, after that post I've seen stuff about a delegator look up and that wouldn't work with my concept. That's why I removed it a few days ago.

Secondly, after some research I found out they are not interested until a real PSR is out: https://github.com/silexphp/Pimple/issues/162.

[mnapoli]

https://github.com/silexphp/Pimple/issues/162 is a year old, maybe their POV of view has changed since (just hoping :) ).

Regarding making a Pimple 3 version that's compatible with container-interop, see that PR: moufmouf/pimple-interop#1 (comment) The conclusion was we need to rewrite Pimple. It doesn't necessarily mean forking and creating a new package, it could be simply extending the base Pimple class. But it probably involves re-implementing all the methods.

(I'm a little late here, sorry for reviving the thread)

[Sam-Burns]

Hi guys, sorry to butt in if this isn't relevant, but would a project like https://github.com/Sam-Burns/pimple3-containerinterop be of any interest to you? \cc @mnapoli

[weierophinney]

Yes, yes it would, @Sam-Burns .

I think it can even be a drop-in replacement for what we have in place already. Would you be willing to do a PR against zendframework/zend-expressive-skeleton to change the dependency used for Pimple? I can then do some integration testing to validate. If it looks good, I'll also update the docs in this repo to reflect the change in dependencies.

[Sam-Burns]

@weierophinney Certainly willing to, yes. Will make sure you're on \cc when the PR is ready.

[weierophinney]

@Sam-Burns Thanks!

[Sam-Burns]

Ref. #228

[geerteltink]

Correct me if I'm wrong, but it looks like the pimple3-containerinterop wraps the container in a custom container class. This way it looses the flexibility of pimple, especially when building the container.

Although I like Pimple 3 support, I'd rather see something that stays closer to Pimple. Maybe revert something like this: zendframework/zend-expressive-skeleton@b4a0923

[geerteltink]

This one is actually extending pimple: https://github.com/snapshotpl/pimple-interop

[weierophinney]

@xtreamwayz —

I've been reviewing the approaches in the implementations of both @Sam-Burns and @snapshotpl, and each has issues:

As you note, Sam's doesn't give direct access to the underlying container, which means you lose flexibility around setting up the container. That can be mitigated in a couple of ways:

• The container accepts the pimple instance, which means you can technically configure the pimple instance before passing it to the interop container constructor. Since the bulk of configuration occurs during bootstrapping, this is likely acceptable.
• Sam's interop instance does accept ServiceProvider instances via the addConfig() method, which means you can do a fair bit of configuration after-the-fact.

My principal remaining concerns is that the interop instance is primarily a proxy, which means overhead when you access it (2X method calls in most cases).

With regards to the work by @snapshotpl, I like the simplicity; all it does is extend the class and add the requisite get() and has() methods. This means that, for most purposes, developers can work with it exactly as they would with any Pimple instance. There are a few issues, though:

• The get() implementation should likely be performing a has() before wrapping the retrieval; it's far cheaper than doing a try/catch block for something that will fail.
• The composer.json dep definition for pimple only says @stable. This is a double-edged sword; it allows usage of Pimple 1, 2, or 3, but if a stable version introduces the get() and/or has() methods, there's now a potential for conflict in implementation. Additionally, it means that any integration we do with that package is unstable, as the configuration mechanisms between the versions present a fair number of differences.

Ideally, what I'd like to see is a version that:

• Simply extends Pimple to add the get() and has() methods.
• Pins to a specific major version of Pimple.

The options we have for that are:

• contribute to one of the above solutions, and use it.
• create yet another interop version of pimple that accomplishes those goals.

The first is the only real solution, as far as I'm concerned; I'd really like to avoid NIH syndrome.

The real question I have at this time is whether we should try and accomplish this for the initial stable release. Thoughts?

[geerteltink]

@weierophinney How about this: https://github.com/xtreamwayz/pimple-container-interop

It also tries to implement the delegate lookup feature, although I'm not sure if it's implemented correctly.

[akrabat]

FWiW, Slim extends Pimple to add the container-interop methods: https://github.com/slimphp/Slim/blob/3.x/Slim/Container.php#L247-L282

It also does other things(!), but I agree that extending Pimple is the right approach.

[geerteltink]

@akrabat That's basically what I have without the delegate feature: https://github.com/xtreamwayz/pimple-container-interop

[akrabat]

@xtreamwayz Yeah - just looked at your code. It seems fine other than it's in the wrong root namespace :)

[geerteltink]

@akrabat What should be the right one?

EDIT: If I keep it in that namespace and rename the Container to PimpleInterop, we can use it with only few changes.

[akrabat]

@xtreamwayz : How about Xtreamwayz\ :)

[geerteltink]

Can do :)

[snapshotpl]

No problem guys, I can improve my repo today

[Sam-Burns]

@weierophinney Just out of interest, what is your concern about using the adapter pattern when integrating a third-party library? It's something I would always do, given the opportunity.

If it's about accessing the container directly to set things, maybe the ArrayAccess implementation would mitigate that? If it's more of a performance concern, as executing an additional method call only adds about 2 CPU cycles, I very much doubt it would show up in benchmarking.

The way the docs recommend Pimple be configured, at present, is to use the ArrayAccess to set variables. This will clearly work with Pimple 4+, as long as it still implements ArrayAccess, and if it turns out that the next major Pimple release is Container-Interop compliant, you really should be able to use it directly in exactly the way the adapter is being used in the examples.

I still think using the adapter pattern as standard with third-party libraries is good software architecture; but would you feel better about it if the adapter had a __call() magic function with a passthrough, maybe?

EDIT: This is the passthrough I was referring to. Documenting it now.