Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

Fix implementation of allowEmptyItems in QuietFactory #232

Merged
merged 4 commits into from Aug 27, 2015
Merged

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Aug 25, 2015

Fixes #230

While fixing that bug, I also discovered some flaws with the logging, which I also fix in this PR.

@wouterj wouterj added the ready label Aug 25, 2015
@wouterj wouterj added this to the 2.0 milestone Aug 25, 2015
@wouterj wouterj mentioned this pull request Aug 25, 2015
@wouterj
Copy link
Member Author

wouterj commented Aug 26, 2015

Tests fail because of a bug in Symfony: symfony/symfony#15619

$this->innerFactory->createItem('Home', array('route' => 'not_existent'))
->willThrow('Symfony\Component\Routing\Exception\RouteNotFoundException');

$this->innerFactory->createItem('Home', array())->shouldBeCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

a willReturn and asserting the return value of $factory->createItem would be better than a shouldBeCalled assertion: the important part is not that the inner factory is called, but that the item is returned

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, it may make sense to add another test for allowEmptyItems being false

@dbu
Copy link
Member

dbu commented Aug 26, 2015

👍
i guess we should wait for the symfony bugfix to be merged before we merge this?

@aitboudad
Copy link

it should be fixed now

@dbu
Copy link
Member

dbu commented Aug 26, 2015

@wouterj now the twig deprecations hit us here as well. do you want to add the deprecation env variable to 2.3 and 2.6 as well?

and i guess for the next version after this one, we should drop 2.3 support and actually fix all those deprecations. also requires the sonata upgrade.

@wouterj
Copy link
Member Author

wouterj commented Aug 26, 2015

Updated the PR.

and i guess for the next version after this one, we should drop 2.3 support and actually fix all those deprecations. also requires the sonata upgrade.

Yes, that's what we decided to do when Symfony 3.0 is released.

@lsmith77
Copy link
Member

needs a rebase

@wouterj
Copy link
Member Author

wouterj commented Aug 26, 2015

Rebased

lsmith77 added a commit that referenced this pull request Aug 27, 2015
Fix implementation of allowEmptyItems in QuietFactory
@lsmith77 lsmith77 merged commit 94c9460 into master Aug 27, 2015
@lsmith77 lsmith77 removed the wip/poc label Aug 27, 2015
@lsmith77 lsmith77 deleted the issue_230 branch August 27, 2015 06:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug in QuietFactory
5 participants