Adding PSR-11 methods, exception #216

Open
wants to merge 4 commits into
from

Projects

None yet

7 participants

@Sam-Burns

No description provided.

Sam Burns Adding PSR-11 methods, exception
ab15c90
@lyrixx

The "provide" section of composer.json should be added too.

src/Pimple/Container.php
+ /**
+ * @param string $id
+ *
+ * @return mixed
@lyrixx
lyrixx Feb 16, 2017 Contributor

you should use inheritDoc

src/Pimple/Container.php
+ */
+ public function get($id)
+ {
+ if (!$this->has($id)) {
@lyrixx
lyrixx Feb 16, 2017 Contributor

you can call directly offsetExists

@stof
Contributor
stof commented Feb 16, 2017

IMO, offsetGet should throw the same exception than get for undefined indexes, so that all accesses to the container are consistent.
This would not even be a BC break, as you made NotFoundException extend \InvalidArgumentException which is used today.

Btw, this would simplify get() as it would forward the call directly.

@hkdobrev
Contributor

I think the code should be reversed. has() should check $this->keys and offsetExists() should call has(). Same thing for get() and offsetGet() more or less.

@hkdobrev
Contributor

Also, is there any agreement on how to proceed with that in relation to Silex? I guess this would be released as a new major version of Pimple (otherwise Silex would need to release a new version capping the version constraint for Pimple). However, is there an agreement that a new Silex major version would be launched supporting the PSR-11 compliant Pimple and at the same time registering GET routes in some way. Thanks!

composer.json
- "php": ">=5.3.0"
+ "php": ">=5.3.0",
+ "psr/container": "^1.0",
+ "phpunit/phpunit": "5.0.0"
@hkdobrev
hkdobrev Feb 17, 2017 Contributor

phpunit/phpunit should be in require-dev instead.

@Sam-Burns
Sam-Burns Feb 17, 2017

That shouldn't be there at all, I'll take it out

@stof
Contributor
stof commented Feb 17, 2017

@hkdobrev Silex advocates using the ArrayAccess API, and all existing Pimple code uses it too (obviously, as it is the only API until now). So we should rather not add an extra method call for each container access for them IMO.

composer.json
- "php": ">=5.3.0"
+ "php": ">=5.3.0",
+ "psr/container": "^1.0",
+ "phpunit/phpunit": "5.0.0"
@stof
stof Feb 17, 2017 Contributor

Pinning the 5.0.0 release with an exact match is a total no-go. this version is totally outdated and many bug have been fixed since then.

Thus, the constraint must also allow using PHPUnit 4.8, as testing must go back to PHP 5.3 currently.
Thus, using PHPUnit through composer does not belong to this PR (and was rejected in the past)

@hkdobrev
hkdobrev Feb 17, 2017 Contributor

See #216 (comment)

@Sam-Burns already said he'll take it out. I agree, because this is not relevant to this PR.

@Sam-Burns
Sam-Burns Feb 17, 2017

@stof You're right, it shouldn't be in this PR - added by accident. Will take it out.

@davedevelopment
Contributor

Silex integration will be a difficult one, as Silex\Application currently has a get method for setting up routing.

@stof
Contributor
stof commented Feb 17, 2017

hmm, this means that this cannot be merged in Pimple as is (adding these methods is a BC break)

@hkdobrev
Contributor

There was a similar discussion in #199. That's why I'm asking if there was a discussion and agreement how to proceed with this so both Silex and Pimple can be PSR-11 compliant, but also have a plan for the major releases needed to achieve this.

@stof
Contributor
stof commented Feb 17, 2017

I think what you can do is build a PSR-11 adapter wrapping the Pimple container. anyone needing a PSR-11 container would inject that.

@hkdobrev
Contributor

For me the ideal approach is to release this as Pimple 4.x. Then plan for next major version of Silex a BC break so new GET routes are registered through another method.

@stof
Contributor
stof commented Feb 17, 2017

@hkdobrev this would be a pretty bad BC break for silex, and something which would make the route API of Silex harder to use. So I see only drawbacks for the Silex project.

Sam Burns Changing exception thrown by offsetGet(). Docblocks.
70a0894
@ragboyjr

@stof what if pimple added a method to wrap itself in the PsrWrapper?

$c = new Pimple\Container([
  'service' => function() {}
]);
$psr_container = $c->toPsr();

And toPsr would just simply look like:

return new PsrPimpleWrapper($this);

I guess another thing we can do is automatically is something like what @GromNaN did in his pimple container interop library where it adds a new service entry as container which is a psr compatible container.

@xtreamwayz

I think what you can do is build a PSR-11 adapter wrapping the Pimple container. anyone needing a PSR-11 container would inject that.

You mean like these?

https://github.com/moufmouf/pimple-interop
https://github.com/Sam-Burns/pimple3-containerinterop
https://github.com/xtreamwayz/pimple-container-interop
https://github.com/GromNaN/pimple-interop

@ragboyjr

@xtreamwayz precisely, thanks for pulling up those reference. @GromNaN's version i think would be the one we'd want to implement in this repo because it's a proper wrapper of the Pimple container to the ContainerInterface.

@hkdobrev
Contributor

@stof I just don't want Pimple to become PSR-11 compliant without Silex becoming compliant as well. If there is a wrapper around Pimple without Silex knowing about it, there's no much sense for me.

@Sam-Burns Sam-Burns Creating separate PSR-11 container
5b0ee21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment