Skip to content

Fix route name resolving with subresources #1110

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 22, 2017
Merged

Fix route name resolving with subresources #1110

merged 1 commit into from
May 22, 2017

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented May 10, 2017 •
edited
Loading

Also resolves conflicts with existing subresources on a property having
the same name

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? behat is okay, unit tests are nogo
Fixed tickets #1107
License MIT
Doc PR ma

Unit tests don't pass for now, waiting for an answer to the below comment.

soyuka
soyuka commented May 10, 2017
View reviewed changes
src/Bridge/Symfony/Routing/RouteNameResolverInterface.php Outdated
@@ -32,5 +32,5 @@
*
* @return string
*/
public function getRouteName(string $resourceClass, $operationType): string;
public function getRouteName(string $resourceClass, $operationType, array $context = []): string;
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 think that this is a breaking change. Should I avoid changing this interface? I need more informations when retrieving the route name for a subresource because multiple routes may find a wrong match on the same resource class. It has to take into consideration the subresource tree. See isSameSubresource in RouteNameResolver.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is a BC break.

@soyuka soyuka requested a review from teohhanhui May 10, 2017 09:35
teohhanhui
teohhanhui reviewed May 11, 2017
View reviewed changes
tests/Fixtures/TestBundle/Entity/DummyTour.php Outdated
* @ApiResource
* @ORM\Entity
*/
class DummyTour
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest renaming this to DummyProduct (Tour is from my own project), since the offers property is defined by https://schema.org/offers

@soyuka soyuka force-pushed the fix/1107 branch 2 times, most recently from e962113 to df0c44d Compare May 11, 2017 10:19
@soyuka
Copy link
Member Author

soyuka commented May 11, 2017

I've done some dirty fix to avoid breaking, let me know your thoughts or if you have another idea on how to do this.
Obviously, phpstan won't be happy with this:

 ------ ----------------------------------------------------------------------------------
  Line   tests/Bridge/Symfony/Routing/RouteNameResolverTest.php
 ------ ----------------------------------------------------------------------------------
  135    Method ApiPlatform\Core\Bridge\Symfony\Routing\RouteNameResolver::getRouteName()
         invoked with 3 parameters, 2 required.
 ------ ----------------------------------------------------------------------------------

@soyuka soyuka added the bug label May 15, 2017
@teohhanhui
Copy link
Contributor

Hehe... We can ignore the PHPstan error for now. 😄

@soyuka soyuka force-pushed the fix/1107 branch 2 times, most recently from 460e2b7 to 59afc11 Compare May 22, 2017 08:27
@soyuka
Copy link
Member Author

soyuka commented May 22, 2017

ping @dunglas is also good to merge if you agree with the routename patch!

dunglas
dunglas requested changes May 22, 2017
View reviewed changes
src/Bridge/Symfony/Routing/CachedRouteNameResolver.php Outdated
@@ -51,7 +51,7 @@ public function getRouteName(string $resourceClass, $operationType): string
// do nothing
}

$routeName = $this->decorated->getRouteName($resourceClass, $operationType);
$routeName = $this->decorated->getRouteName($resourceClass, $operationType, @func_get_arg(2));
Copy link
Member

Choose a reason for hiding this comment

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

Can you do it like that instead https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Serializer/Serializer.php#L182-L190

src/Bridge/Symfony/Routing/RouteNameResolver.php Outdated
{
$context = @func_get_arg(2);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

done thanks

Fix route name resolving with subresources
e66ed60 
Also resolves conflicts with existing subresources on a property having
the same name
@dunglas dunglas merged commit 183b0a5 into api-platform:master May 22, 2017
@dunglas
Copy link
Member

dunglas commented May 22, 2017

Thanks @soyuka

@soyuka soyuka deleted the fix/1107 branch May 22, 2017 12:51
@soyuka soyuka mentioned this pull request May 27, 2017
Closed
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
@dunglas
Merge pull request api-platform#1110 from soyuka/fix/1107
883fcf1 
Fix route name resolving with subresources
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teohhanhui teohhanhui teohhanhui left review comments

@dunglas dunglas dunglas requested changes

Assignees
No one assigned
Labels
bug [RFR] Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soyuka @teohhanhui @dunglas