Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.x Remove Pimple #2587

Closed
wants to merge 7 commits into from
Closed

4.x Remove Pimple #2587

wants to merge 7 commits into from

Conversation

l0gicgate
Copy link
Member

Removing Pimple as per discussion in #2585.

@coveralls
Copy link

coveralls commented Feb 22, 2019

Coverage Status

Coverage remained the same at 98.487% when pulling b1cb7c7 on l0gicgate:4.x-RemovePimple into a2f22e1 on slimphp:4.x.

@l0gicgate l0gicgate added this to the 4.0 milestone Feb 22, 2019
l0gicgate added a commit to l0gicgate/Slim that referenced this pull request Feb 28, 2019
@l0gicgate l0gicgate closed this Mar 2, 2019
@l0gicgate l0gicgate deleted the 4.x-RemovePimple branch March 2, 2019 15:52
@l0gicgate l0gicgate restored the 4.x-RemovePimple branch March 2, 2019 15:56
@l0gicgate l0gicgate reopened this Mar 2, 2019
@@ -39,8 +37,7 @@ public static function setupBeforeClass()

public function testGetContainer()
{
$pimple = new Pimple();
$container = new Psr11Container($pimple);
$container = new MockContainer();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have a MockContainer object when we could be using PHPUnit's createMock() ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have ArrayAccess implemented on the class unless you want to force usage of the ContainerInterface::set() method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have ArrayAccess implemented on the class

Why? Slim doesn't set anything into the container anymore does it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread your question. We can mock a container for that specific test sure. We have a MockContainer though so why not use it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have a MockContainer though? We shouldn't need it as we should be able to use PHPUnit's Mocking functionality for every situation where Slim's classes use a container. If we can't then we need to look at that particular Slim code.

@l0gicgate
Copy link
Member Author

Will refactor with Mocks instead and re-open.

@l0gicgate l0gicgate closed this Mar 2, 2019
@l0gicgate l0gicgate deleted the 4.x-RemovePimple branch April 2, 2019 19:03
@l0gicgate l0gicgate mentioned this pull request Apr 25, 2019
@l0gicgate l0gicgate mentioned this pull request Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants