On 10/26/2016 08:28 AM, Matthew Weier O'Phinney wrote:
Hello, everyone!
PSR-11, Container Interface (née container-interop) has been in incubation for a
couple years now, and the editor and sponsors feel it is ready for review. As
coordinator, I hereby open the mandatory review period prior to a formal
acceptance vote; voting will begin no earlier than 13:30 UTC on Wednesday,
November 9th, 2016.
For the current version, please reference:
-
https://github.com/php-fig/fig-standards/blob/a47c644f9d0f914bb0a9777eeaec157f2d51dbff/proposed/container.md
-
https://github.com/php-fig/fig-standards/blob/a47c644f9d0f914bb0a9777eeaec157f2d51dbff/proposed/container-meta.md
The related library containing the interfaces is here:
- https://github.com/php-fig/container
Additionally, a large number of projects either implement or consume
container-interop already, and would be (relatively) immediately compatible:
- https://github.com/container-interop/container-interop#compatible-projects
Please familiarize yourself with the specification and its scope, and reply on
this thread with any concerns.
Thanks!
Other thoughts, in a separate reply as they're not directly related to
the other discussion, not really in any special order:
* The spec should specify what legal characters are for an entry
identifier, and what if any reserved characters there are. It should
also specify minimum supported key length and character sets, for
completeness. I recommend borrowing PSR-6's language here, which I
believe addresses this area well. Whether it uses the same reserved
character list or another one I don't feel strongly about. I would not
consider this a Review-breaking change.
* The allowance for additional parameters on get() is worrisome. That
implies ANY use of a second or further parameter sets me up for an
incompatibility, but implementations are free to add those
implementation-specific hooks that I can couple to. I would much prefer
to omit that allowance entirely. (Technically optional parameters
cannot be prevented, but we shouldn't encourage it.)
* What other exceptions might get($id) throw besides NotFoundException?
* I would much prefer to see a DependencyNotFoundException defined for
that case than leaving that up to individual implementers. Standard
error handling is just as important if not moreso than the happy path.
* The text of the spec says that the psr/container package doesn't exist
yet, but Matthew's post above links to it. :-)
* Section 4 of the metadoc: I like! Thank you.
* I don't believe any other spec put exceptions into their own
sub-namespace. That seems unnecessary to me, and I'd prefer to remove
that sub-namespace and just leave the exceptions in Psr/Container.
There's no reason to have this inconsistency.
* In metadoc 7.3.2, there's this line:
throw NotFoundException::entryNotFound($identifier);
Which seems to come out of nowhere. The interface doesn't define a
static method factory. The hypothetical MissingDependencyException
later in the method just uses the constructor. The example here should
do the same and use the constructor.
Now for the big one:
Metadoc 8.2 seems redundant with section 1.4 of the spec, as in, nearly
identical. It doesn't add anything so should just be removed. Assume
someone reading the metadoc has already read the spec and has questions
about it.
The section on delegate containers strikes me as very odd. It describes
behavior that is... inherently possible with any interface of almost any
kind. Composing other objects of the same interface is one of the core
features of interfaces, so describing it here is almost entirely
redundant, especially when no actual mechanism for such composition is
defined. I understand that such composition is a key feature of the
spec, but the spec doesn't actually define how to do it other than "ya
know, it's possible because interfaces". That seems needlessly redundant.
What it does seem to define is that delegates should be looked up
exclusively for indirect dependencies. Which... does not make any sense
to me at all.
Reading both section 1.4 of the spec and 8 of the metadoc, I am still
confused about how I would "properly" implement the delegation concept
at all. The example (with the pictures) assumes the composite container
is empty of its own services, and is just delegating in order to other
containers. The spec, however, says that get() should operate on self,
but dependencies on child containers. That implies the composite
container has its own services. The metadoc discussion (8.7) talks as
though that's temporary until the rest of the market catches on and gets
around to just providing empty composite containers... but then the spec
still doesn't tell me what I'm supposed to do in this case.
Moreover, the spec and metadoc are inconsistent. The spec says that a
container must
- provide some way to specify child (delegate) containers (which could
vary by implementation)
- call a direct-get() on self only
- call a dependency-get() on child only
- call has() on self only
- But maybe will dependency-get() on self, if they really mean it
This last point is highly troublesome as it makes behavior unpredictable.
However, the metadoc in section 8.3 says the expected usage is:
- A composite container (new term) forwards all get() calls to its child
containers until one is found
- A non-composite container forwards all dependency-get() calls to
its... parent composite container
- Loop until done
That's not at all the same logic. As a potential implementer, I have no
idea what I'm supposed to do if my composite container has its own
services. Do I forward them or not? Also, can any container be a
composite container or no? Can a composite container be nested inside
another? If so, to where do I forward get() calls?
The spec refers only to a delegate container, but I'm using the terms
parent and child. That's because when you have a reference from one
container to another, that's a parent-child relationship (object with
the reference to the object being referenced). What it appears needs to
happen for what the metadoc describes (which is not what the spec
describes) is for the composite container to have a reference to all of
its sub-containers, and then the sub-containers to also have a reference
back to it in order to delegate their dependency-get() calls. That
creates a circular reference. While those are not the memory leak they
once were in PHP, circular references should almost always be seen as a
code smell. That I do not know how to implement this pattern without
them is a sign that something is very wrong.
Moreover, it seems to suggest that I cannot nest containers
indefinitely. If Container2 has multiple delegate containers defined,
then... what happens? It must, or else CompositeContainer is
fundamentally different than a Container, but it's not defined in the spec.
Now, consider the case where container2 delegates to composite container
but the service it's looking for doesn't exist anywhere.
CompositeContainer would have to call has() on each sub-container in
turn, and then call get() if found. But has() specifically doesn't
delegate, so they can't have nested containers. So, nested containers
aren't really banned, just not actually possible to implement, but
that's not stated.
Moreover, all of this logic seems to then exist in get() or sub-calls
therein. That makes get() considerably more expensive with multiple
internal calls and branch points. From Drupal I know that the
container's lookup process is very much along the critical path of the
application; adding even one method call to a get() lookup results in,
for our case, hundreds of additional stack calls (not cheap in PHP) over
the course of a request. That's a performance hit we would be unwilling
to swallow.
At this point I'm completely lost, and write this portion of the spec
off as unimplementable. Clearly there are some implementations that are
doing something with it, as noted in the metadoc. However, from the
spec and metadoc itself, which is all I am voting on (I did not look at
the linked discussion threads either, as those are not what I'm voting
on and the purpose of the metadoc is to summarize those in one place), I
don't know how I'd even implement this correctly, or in a manner that
would let me assemble different containers that didn't know about each
other without intimately knowing about all of them, and even then it
would be creating an object graph with multiple circular references,
which is a code smell.
Quite simply, as written I cannot vote for this spec with the delegate
logic as ill-defined as it is, and the only way I can see to even try to
implement it resulting in such a slow and loopy object and call graph.
I will leave these comments here for response before suggesting any
alternatives.
--Larry Garfield
--
You received this message because you are subscribed to the Google Groups "PHP
Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to php-fig+unsubscr...@googlegroups.com.
To post to this group, send email to php-fig@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/php-fig/8a66ddd5-c4ae-282e-f02d-cbf539c3b714%40garfieldtech.com.
For more options, visit https://groups.google.com/d/optout.
