-
-
Notifications
You must be signed in to change notification settings - Fork 935
hotfix: add exception when the action is not a builtin action #586
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
hotfix: add exception when the action is not a builtin action #586
Conversation
5170551 to
6087d46
Compare
6087d46 to
b6e85b3
Compare
b6e85b3 to
6b32716
Compare
|
👍 |
| $containerBuilderProphecy = $this->prophesize(ContainerBuilder::class); | ||
|
|
||
| $kernelProphecy->getContainer()->willReturn($containerBuilderProphecy); | ||
| $containerBuilderProphecy->has(Argument::any())->willReturn(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a more specific wildcard here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument::type('string') indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can limit it to the specific values we need here.
On 27 Jun 2016 21:50, "Kévin Dunglas" notifications@github.com wrote:
In tests/Bridge/Symfony/Routing/ApiLoaderTest.php
#586 (comment):@@ -113,6 +115,10 @@ private function getApiLoaderWithResourceMetadata(ResourceMetadata $resourceMeta
$kernelProphecy = $this->prophesize(KernelInterface::class); $kernelProphecy->locateResource(Argument::any())->willReturn($routingConfig);
$containerBuilderProphecy = $this->prophesize(ContainerBuilder::class);
$kernelProphecy->getContainer()->willReturn($containerBuilderProphecy);$containerBuilderProphecy->has(Argument::any())->willReturn(true);Argument::type('string') indeed
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/api-platform/core/pull/586/files/6b32716eb4e97b5aadd594a29fb7ee625b0490a8#r68579198,
or mute the thread
https://github.com/notifications/unsubscribe/AAhf647GnFhS_HyQaZz6cQNKgae5Pvahks5qP9VDgaJpZM4I-_Cx
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @teohhanhui said, we expect specific values here don't we? So we should check against them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use both since I want to test that the exception is triggered correctly
63790ee to
8b9b7f9
Compare
|
Thanks @teohhanhui @theofidry @dunglas for reviewing this PR :) |
| ]; | ||
|
|
||
| $containerInterfaceProphecy = $this->prophesize(ContainerInterface::class); | ||
| $containerInterfaceProphecy->reveal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$containerProphecy = $this->prophesize(ContainerInterface::class);
// ...
$kernelProphecy->getContainer()->willReturn($containerProphecy->reveal());There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or:
$containerProphecy = $this->prophesize(ContainerInterface::class);
/** @var ContainerInterface $container */
$container = $containerProphecy->reveal();
// ...
$kernelProphecy->getContainer()->willReturn($container);if you want to benefit from the typehint and avoid raising a warning on Scrutinizer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theofidry I know @dunglas does not like extra variables... 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not pleased with it either but there is no workaround, to avoid that and benefit from the typehint there is phpSpec for that. But I don't like forcing the users to use phpSpec and you loose your code coverage as well...
Anyway let's blame @dunglas tastes in the meantime 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer PHPSpec too, and I use it for my private projects. But I'm tired of "I don't know PHPSpec so I let you write tests." or "I dont't know PHPSpec so I wrote PHPUnit tests." comments ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I don't know PHPSpec so I let you write tests." or "I don't know PHPSpec so I wrote PHPUnit tests."
The problem isn't here, it's "I won't contribute because I'm not willing to learn a new testing tool just for this project". It's arguable, but that's the reason why we stayed to phpunit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we agree then that there shall be no inline ->reveal()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going for it ;)
02e1dcf to
0349711
Compare
|
comments addressed |
|
👍 |
|
Thank you @Simperfit |
We should not use POST in itemOperation (it is not supported) and each unsupported actionName should not be able to be used since it break NelmioApiDoc.
Thanks to @dunglas for the help.