WIP: Yii2 implements PSR-11 #14107

SamMousa · 2017-05-03T15:25:52Z

Q	A
Is bugfix?	no
New feature?	yes
Breaks BC?	no
Tests pass?	yes

PSR-11 is accepted and Yii implements it.

cebe · 2017-05-04T06:23:07Z

NotFoundExceptionInterface and ContainerExceptionInterface also need to be considered to be compatible. There is more to it than just implementing the interface, behavior must match the spec too:

https://github.com/php-fig/fig-standards/blob/10bb43b1802c0427f8a4a5d1e6a84da83fa7724d/accepted/PSR-11-container.md#12-exceptions

SamMousa · 2017-05-04T07:07:20Z

Will look into that.

…

On May 4, 2017 9:00 AM, "Carsten Brandt" ***@***.***> wrote: NotFoundExceptionInterface and ContainerExceptionInterface also need to be considered to be compatible. There is more to it than just implementing the interface: https://github.com/php-fig/fig-standards/blob/ 10bb43b1802c0427f8a4a5d1e6a84da83fa7724d/accepted/PSR-11- container.md#12-exceptions — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14107 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAhYzfihRJ5DBbhD89F36hi8wWiUhQcpks5r2W7dgaJpZM4NPkJC> .

Faryshta · 2017-05-04T07:26:38Z

the signatures are not the same either

public function get($class, $params = [], $config = [])

public function get($id)

and they do different things too. while get() in psr-11 finds things already in the container, the get() method might build a new object, execute a callback, and if the first parameter is a class name then there is an implied set().

i don't think psr-11 is for yii2

cebe · 2017-05-04T07:46:48Z

the signatures are not the same either

$params and $config are optional, so the interface should be fine.

and they do different things too

if it matches the spec of PSR-11 that is fine, it can do more but it must support what is defined in the PSR to be compatible to it.

SamMousa · 2017-05-04T08:04:15Z

@samdark I'm working on this now.
While looking at the exceptions I noticed this in Container::get(): link

    throw new InvalidConfigException('Unexpected object definition type: ' . gettype($definition));

However this is part of a path that is unreachable. All definitions are validated when they are set inside the container, so this seems like a redundant check on a private variable.
Can I remove it?

Also, NotInstantiableException, something I added in 2.0.9 extends from InvalidConfigException, in hindsight this does not make much sense, can we "schedule" to change this in 2.1.0?
Not sure what the best base class is, but one candidate would be InvalidCallException.

SamMousa · 2017-05-04T11:11:53Z

@Faryshta I think we're fine with respect to get() implementation, this is from the spec:

get takes one mandatory parameter: an entry identifier, which MUST be a string.
get can return anything (a mixed value), or throw a NotFoundExceptionInterface if the identifier
is not known to the container. Two successive calls to get with the same
identifier SHOULD return the same value. However, depending on the implementor
design and/or user configuration, different values might be returned, so
user SHOULD NOT rely on getting the same value on 2 successive calls.

While we support other argument types, users of the container will only pass in strings (since that is what the specs tell them to do), so other behavior is not relevant for them.
Our DI container might not only return singletons, but this is a recommendation (SHOULD) not a hard requirement (MUST). Furthermore, calling code SHOULD NOT assume that we always return singletons.

wggwcn · 2017-05-05T00:02:53Z

watching it

dynasource · 2017-05-08T23:16:50Z

framework/di/InvalidConfigException.php

+
+namespace yii\di;
+
+use \yii\base\InvalidConfigException as Base;


using Base as alias is not preferable. Using the full class name is better here

Why not implement ContainerExceptionInterface at yii\base\InvalidConfigException after all, the init() method on most classes throws that exception already.

yii\base\InvalidConfigException has a wider meaning - it refers to any mis-configuration error, which is not mandatory triggered by DI.

So? The PSR never says that only DI can throw said exceptions.

Besides as you say any configuration error will throw yii\base\InvalidConfigException which by the PSR.

A call to the get method with a non-existing id MUST throw a Psr\Container\NotFoundExceptionInterface.

So $di->get('db', [/* invalid params or config */]) must throw Psr\Container\NotFoundExceptionInterface

dynasource · 2017-05-08T23:18:59Z

composer.json

@@ -78,7 +78,8 @@
        "bower-asset/jquery": "2.2.*@stable | 2.1.*@stable | 1.11.*@stable | 1.12.*@stable",
        "bower-asset/jquery.inputmask": "~3.2.2 | ~3.3.3",
        "bower-asset/punycode": "1.3.*",
-        "bower-asset/yii2-pjax": "~2.0.1"
+        "bower-asset/yii2-pjax": "~2.0.1",
+        "psr/container": "^1.0"


I'd rather not pollute composer.json for such 'nice-to-have' features

That's fine. It's an interface and there's no other way to solve it rather than requiring it.

samdark · 2017-06-25T23:01:43Z

However this is part of a path that is unreachable. All definitions are validated when they are set inside the container, so this seems like a redundant check on a private variable.
Can I remove it?

Yes.

samdark · 2017-06-25T23:04:03Z

Also, NotInstantiableException, something I added in 2.0.9 extends from InvalidConfigException, in hindsight this does not make much sense, can we "schedule" to change this in 2.1.0?
Not sure what the best base class is, but one candidate would be InvalidCallException.

What would extend PSR NotFoundExceptionInterface then?

samdark · 2017-06-25T23:04:59Z

Overall signatures and behavior are compatible. I see no reason not to merge it.

SamMousa · 2017-06-26T06:57:15Z

What would extend PSR NotFoundExceptionInterface then?

NotInstantiableException will still implement it. It just won't be derived from InvalidConfigException (since it is not invalid config, but an invalid call that is causing it). This is entirely optional and mostly academical ;-)

SamMousa · 2017-06-26T08:17:31Z

@samdark Could you check why the tests are not working? I've done a reinstall of composer.lock since it had a merge conflict. Now it seems composer install is failing for all tests.

Side note: do we really need composer.lock in our repository? Does it even do anything?
https://getcomposer.org/doc/02-libraries.md#lock-file

samdark · 2017-06-26T12:13:31Z

Probably lock file is in damaged state. Delete it, re-run composer update (ideally w/ PHP 5.4) and commit changes. We need it to run code coverage and code analysis tools. They're not able to install global composer plugins.

SamMousa · 2017-06-26T12:21:50Z

Where do i get PHP5.4? Wayback Machine? :-D

This was originally a joke, I thought there would be an official docker image with PHP5.4.... Turns out I was wrong

samdark · 2017-06-26T12:24:37Z

php.net :) I can take care of it.

SamMousa · 2017-06-26T12:25:06Z

Okay, great, google gave me this docker image: https://github.com/alterway/docker-php/blob/master/doc-php-cli.md

samdark · 2017-06-26T12:27:21Z

These would do as well.

SamMousa · 2017-06-26T12:36:45Z

@samdark I have reinstalled all using a php 5.4 environment, still doesn't seem to be working.

samdark · 2017-06-26T21:22:52Z

composer.lock.zip

SamMousa · 2017-06-27T08:14:45Z

@samdark I give up; it seems the composer.lock you posted results in a wrong phpunit version... I'd appreciate it if you could fix it 8-)

samdark · 2017-06-27T11:25:01Z

Locally I did. Container unit tests are giving errors...

cebe · 2017-07-12T10:04:26Z

Overall signatures and behavior are compatible. I see no reason not to merge it.

from the spec: If has($id) returns false, get($id) MUST throw a NotFoundExceptionInterface.

The container currently does not have this behavior. We instantiate classes even if there is no definition and has() returns false on them.

However, the description of the has() method in the interface is not as strict:

has($id) returning true does not mean that get($id) will not throw an exception.
It does however mean that get($id) will not throw a NotFoundExceptionInterface.
from the spec: A call to the get method with a non-existing id MUST throw a Psr\Container\NotFoundExceptionInterface.

I do not see where this is implemented right now.
from the spec: Packages providing a PSR container implementation should declare that they provide psr/container-implementation 1.0.0.

a "provide" section should be added to the composer.json in / as well as in /framework

SamMousa · 2017-07-12T11:19:51Z

@cebe Can you double check the exception stuff? Note that NotInstantiableException extends the PSR exception.

klimov-paul · 2017-09-07T12:26:54Z

Applying ContainerInterface over existing Container produces potential interface compatibility error.
At the ContainerInterface get() and has() methods have first parameter named $id, while same methods of Container use $class name. Compare:

interface ContainerInterface
{
     public function get($id)
}

with:

class Container extends Component
{
    public function get($class ...)
    {
    }
}

While during inheritance change of the method argument is allowed, it may end with E_STRICT error at some environment. We already have similar issues like #13538 about slight inconsistencies around method redeclaration, which can be spoted only in particular strict environment.

To make things right signature of the Container methods should be changed like following:

class Container extends Component
{
    public function get($id, $params = [], $config = [])
    {
    }
}

which actually a BC break.

SamMousa · 2017-09-07T12:33:52Z

@klimov-paul as far as I know the parameter name is different, the type is not.
They are both string identifiers for something that you want from your container.
Doing $container->get(SomeClass::class); in no way guarantees the type of object. Using the class name as identifier is just a common convention that works well.

klimov-paul · 2017-09-07T12:35:57Z

I can not say that current implementation of the Container actually matches ContainerInterface, since ContainerInterface::get() expects to throw NotFoundExceptionInterface in case no definition is found for given $id, while Container::get() attempt to build not existing definition.

At the present state of this PR Container::get() thows NotFoundExceptionInterface only in case definition is interface or abstract class, while this should refer to ContainerExceptionInterface.

klimov-paul · 2017-09-07T12:28:38Z

framework/di/NotInstantiableException.php

@@ -16,7 +17,7 @@
 * @author Sam Mousa <sam@mousa.nl>
 * @since 2.0.9
 */
-class NotInstantiableException extends InvalidConfigException
+class NotInstantiableException extends InvalidConfigException implements NotFoundExceptionInterface


This does not seem to be appropriate: NotFoundExceptionInterface means there is NO definition of the entity inside the container, while NotInstantiableException means the definition is invalid, pointing to something, which can not be intantiated like interface or absract class.
These exeptions can not be made equal.

The PSR does not have something for not instantiable.

klimov-paul · 2017-09-07T12:29:39Z

framework/di/InvalidConfigException.php

+
+namespace yii\di;
+
+use \yii\base\InvalidConfigException as Base;


yii\base\InvalidConfigException has a wider meaning - it refers to any mis-configuration error, which is not mandatory triggered by DI.

klimov-paul · 2017-09-07T12:38:07Z

Doing $container->get(SomeClass::class); in no way guarantees the type of object. Using the class name as identifier is just a common convention that works well.

Sorry, but I do not understand what are you referring to.
Method signature involves both arguments name and type.

klimov-paul · 2017-09-07T12:44:13Z

I suggest applying PSR-11 to be performed at 2.1 branch in appropriate matter, including methods signature change, exception hierarchy revision and logic adjustments, instead of patching 2.0.x by all cost.

SamMousa · 2017-09-07T13:21:43Z

I don't mind waiting for 2.1 for this, just not all your arguments are valid.

It is fine to provide a container that has default definitions for ids that happen to be class names. That does not break the spec, it just means that the container is preconfigured.

I wasn't aware that the name of an argument was relevant; if it is that's something that can be easily fixed of course.

samdark · 2017-09-07T18:50:48Z

I agree that it makes sense to do the change in 2.1 where we can actually adjust semantics correctly, add features necessary and change method signatures. I'm closing the pull request. Let's use #14112 for future discussion.

Yii2 implements PSR-11

5ea4d02

samdark self-assigned this May 3, 2017

samdark added the type:enhancement label May 3, 2017

samdark added this to the 2.0.13 milestone May 3, 2017

Container exceptions now implement PSR-11 interfaces

ee198a6

SamMousa mentioned this pull request May 4, 2017

[2.1] Make Container PSR-11 compatible, support different PSR-11 DI containers #14112

Closed

3 tasks

dynasource suggested changes May 8, 2017

View reviewed changes

dynasource reviewed May 8, 2017

View reviewed changes

samdark approved these changes Jun 17, 2017

View reviewed changes

samdark requested review from cebe, klimov-paul and SilverFire June 25, 2017 22:56

Fixed merge conflicts. Cleaned up Container::get().

f13d4ee

Composer install with php 5.4

b6be02e

Fixed composer.lock

4b54f9c

update composer.lock

171c77c

klimov-paul suggested changes Sep 7, 2017

View reviewed changes

samdark closed this Sep 7, 2017

cebe removed this from the 2.0.13 milestone Sep 8, 2017


		namespace yii\di;

		use \yii\base\InvalidConfigException as Base;

WIP: Yii2 implements PSR-11 #14107

WIP: Yii2 implements PSR-11 #14107

Conversation

SamMousa commented May 3, 2017 • edited Loading

cebe commented May 4, 2017 • edited Loading

SamMousa commented May 4, 2017 via email

Faryshta commented May 4, 2017

cebe commented May 4, 2017

SamMousa commented May 4, 2017

SamMousa commented May 4, 2017

wggwcn commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Faryshta Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdark commented Jun 25, 2017

samdark commented Jun 25, 2017

samdark commented Jun 25, 2017

SamMousa commented Jun 26, 2017 • edited Loading

SamMousa commented Jun 26, 2017

samdark commented Jun 26, 2017

SamMousa commented Jun 26, 2017 • edited Loading

samdark commented Jun 26, 2017

SamMousa commented Jun 26, 2017

samdark commented Jun 26, 2017

SamMousa commented Jun 26, 2017

samdark commented Jun 26, 2017

SamMousa commented Jun 27, 2017

samdark commented Jun 27, 2017

cebe commented Jul 12, 2017 • edited Loading

SamMousa commented Jul 12, 2017

klimov-paul commented Sep 7, 2017

SamMousa commented Sep 7, 2017

klimov-paul commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimov-paul commented Sep 7, 2017 • edited by samdark Loading

klimov-paul commented Sep 7, 2017

SamMousa commented Sep 7, 2017

samdark commented Sep 7, 2017

SamMousa commented May 3, 2017 •

edited

Loading

cebe commented May 4, 2017 •

edited

Loading

Faryshta Sep 7, 2017 •

edited

Loading

SamMousa commented Jun 26, 2017 •

edited

Loading

SamMousa commented Jun 26, 2017 •

edited

Loading

cebe commented Jul 12, 2017 •

edited

Loading

klimov-paul commented Sep 7, 2017 •

edited by samdark

Loading