[ParamConverter] Unable to guess how to get a Doctrine instance from the request information #21563

Closed
Belgacem-TLILI opened this Issue Feb 8, 2017 · 24 comments

Projects

None yet

6 participants

@Belgacem-TLILI
Belgacem-TLILI commented Feb 8, 2017 edited
Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC?
Symfony version 3.2.3

This happen after the last update from Symfony 3.2.2 to 3.2.3
In my action i have multiple routes like below


 /**
     * @Route("/my-route", name="first_route", defaults={"type" = null})
     * @Route("/my-route/report/{objectId}/{startDate}/{endDate}", name="second_route", defaults={"type" = null})
     * @Route("/my-route/{objectId}/{startDate}/{endDate}/{type}", name="third_route", defaults={"type" = null})
     * @ParamConverter(name="object", Class="MyBundle:MyObjectClass")
     */
    public function indexAction(Request $request, $object = null, $startDate = null, $endDate = null, $type)
    {
// Logic
}

if i try to access to the page with the route /my-route i get the below exception

screen shot 2017-02-08 at 10 11 18 am

@iltar
Contributor
iltar commented Feb 8, 2017

Can you add the startDate and endDate to your defaults with null and see if that works?

/cc @chalasr

@chalasr
Contributor
chalasr commented Feb 8, 2017

@Belgacem-TLILI If the given solution doesn't help, we'll need more information:
Is the code snippet exact, or is it something generic adapted for opening this issue? What is the name of the parameter that can't be converted (hidden in the screenshot)? Do you have any issue when browsing other routes defined on your action (if the answer is yes, please describe them as well)?

The best would be to create a fork of the standard edition including the changes necessary to reproduce. Thanks.

@Belgacem-TLILI
Belgacem-TLILI commented Feb 9, 2017 edited

@iltar /cc @chalasr
The code snippet it is adapted for opening the issue,
i'm using the first route to access to my page, the name of the parameter that can't be converted is object
i fixed the issue by adding isOptional="true" to the ParamConverter


 /**
     * @Route("/my-route", name="first_route", defaults={"type" = null})
     * @Route("/my-route/{objectId}/{startDate}/{endDate}", name="second_route", defaults={"type" = null})
     * @Route("/my-route/{objectId}/{startDate}/{endDate}/{type}", name="third_route", defaults={"type" = null})
     * @ParamConverter(name="object", Class="MyBundle:MyObjectClass", isOptional="true")
     */
    public function indexAction(Request $request, $object = null, $startDate = null, $endDate = null, $type)
    {
// Logic
}
@Belgacem-TLILI

This was working before the update to symfony 3.2.3 and i can access to my page using the 3 routes without any issue.

@xabbuh
Member
xabbuh commented Feb 9, 2017

Which version of SensioFrameworkExtraBundle do you have?

@xabbuh
Member
xabbuh commented Feb 9, 2017

To add to my question: Can you check if your issue is caused by sensiolabs/SensioFrameworkExtraBundle@bd7e329 which was fixed by sensiolabs/SensioFrameworkExtraBundle@ed86f6f (and is part of version 3.0.21).

@Belgacem-TLILI

@xabbuh Thank you: We have SensioFrameworkExtraBundle version 3.0.21 but we don't have this commit sensiolabs/SensioFrameworkExtraBundle@ed86f6f

@xabbuh
Member
xabbuh commented Feb 9, 2017

This looks weird. Can you reinstall SensioFrameworkExtraBundle?

@Belgacem-TLILI

@xabbuh ive reinstalled SensioFrameworkExtraBundle but i still don't have the commit

screen shot 2017-02-09 at 2 22 53 pm

screen shot 2017-02-09 at 2 23 15 pm

@xabbuh
Member
xabbuh commented Feb 9, 2017

But the commit is there (see the reference attribute in the composer.lock file as well as the contents of the PHP file.

@Belgacem-TLILI
Belgacem-TLILI commented Feb 9, 2017 edited

@xabbuh sorry i misunderstand, the issue is coming from that commit i was thinking the commit was reverted
if i remove the false parameter from DoctrineParamConverter it works like before the migration to 3.2.3

screen shot 2017-02-09 at 3 32 18 pm

@chalasr
Contributor
chalasr commented Feb 9, 2017

It seems to be a remaining side effect from the changes made in #21333 and #21379.
Before the param converter was relying on a request attribute that was automatically created for the $object argument, so the param converter was calling setOptional(true). Now the request attribute is not created anymore (it was causing an annoying bug on argument resolvers) so the call doesn't occur.

AFAIK fixing it would require to use reflection in order to add a || ($argument->isDefaultValueAvailable() && null === $argument->getDefaultValue()) to the existing check. I'll try to fix it that way on the bundle if no objection.

@iltar
Contributor
iltar commented Feb 9, 2017

@chalasr does that include the php 7.1 nullable? Because that's pretty much his signature due to a non-optional argument as last.

@chalasr
Contributor
chalasr commented Feb 9, 2017 edited

@iltar The previous behavior was not supporting nullable types (chalasr@d3fa8a1#diff-90cbecfd4505e6e8af5842db11add3b5L142) but only default values including null. I think we can fix it for that so it works as expected, and add support for the nullable type in a next time (if we want to improve param converters of course :) ).

Btw the param converter already uses reflection for another topic and does not seem to handle nullable type either: https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/3.0/Request/ParamConverter/DoctrineParamConverter.php#L195

Edit: nullable type are supported

@chalasr
Contributor
chalasr commented Feb 9, 2017 edited

@Belgacem-TLILI Could you please try the following patch on your project and tell me if it fixes the issue? chalasr/SensioFrameworkExtraBundle@91bf0c2?w=1

@chalasr
Contributor
chalasr commented Feb 11, 2017
@Belgacem-TLILI

@chalasr no that didn't fix the issue

@chalasr
Contributor
chalasr commented Feb 11, 2017

I updated the patch meantime, is it the one you tried?

@Belgacem-TLILI

@chalasr i tested the below

screen shot 2017-02-11 at 15 23 53

@chalasr
Contributor
chalasr commented Feb 13, 2017

@Belgacem-TLILI Sorry for the fail. It would be nice if you could try this one chalasr/SensioFrameworkExtraBundle@1068c57, it should be ok now.

@chalasr chalasr referenced this issue in sensiolabs/SensioFrameworkExtraBundle Feb 14, 2017
Merged

Controller arguments with default values should be default optional #460

@chalasr
Contributor
chalasr commented Feb 14, 2017

Thank you for confirming. I opened a PR on the bundle.

@Belgacem-TLILI

You are Welcome

@fabpot fabpot closed this Feb 14, 2017
@fabpot fabpot added a commit to sensiolabs/SensioFrameworkExtraBundle that referenced this issue Feb 15, 2017
@fabpot fabpot bug #460 Controller arguments with default values should be default o…
…ptional (chalasr)

This PR was merged into the 3.0 branch.

Discussion
----------

Controller arguments with default values should be default optional

This makes that for e.g:

```php
function fooAction($foo = null, Request $request);
```

the `$foo` parameter is considered optional.

Before it was relying on `ReflectionParameter::isOptional()` only, which returns false if the parameter is not the last. This additionally checks for `isDefaultValueAvailable()`.
To me it's a bug that was hidden by the fact request attributes were automatically created for any parameter with a default value, that has been fixed in symfony.

Fixes symfony/symfony#21563.

Commits
-------

8bd1da3 Controller arguments with default values should be optional
1c66c2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment