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

Adjust Cart\ItemResolverInterface to remove dependency on Symfony Request class #1234

Merged
merged 1 commit into from
Mar 20, 2014

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Mar 19, 2014

After little discussion in #1233 I have decided to follow those suggestion and remove dependency on Request class in ItemResolverInterface.

@pjedrzejewski @patkar What do you think about this change?

@stloyd
Copy link
Contributor Author

stloyd commented Mar 19, 2014

@Sylius What do you think about this new method on interface? It could be removed, but I think it add some clarification into this resolver...

@jjanvier
Copy link
Contributor

Do we need really need resolveItemIdentifier in the interface ? Agree it's useful for us in the CoreBundle but I don't understand what it brings on the CartBundle..

@QuingKhaos
Copy link
Contributor

Hehe, looks good :) It's more a matter of taste, if resolveItemIdentifier should be in the interface.

@pjedrzejewski
Copy link
Member

@patkar It definitely should not be in the interface, because our implementation is just one example of adding item to cart. In my projects, I had resolvers which used plain forms to get the items. (without the id of entity in query) so it's up to the implementation to decide. I think the change Request -> $data should be enough. resolveItemIdentifier should be removed from the interface. It can stay on the ItemResolver in core bundle though.

@QuingKhaos
Copy link
Contributor

👍

@stloyd stloyd added this to the 1.0.0-BETA1 milestone Mar 20, 2014
@stloyd stloyd removed the RFC label Mar 20, 2014
@stloyd
Copy link
Contributor Author

stloyd commented Mar 20, 2014

@pjedrzejewski Updated. Failed tests are not related to this.

pjedrzejewski pushed a commit that referenced this pull request Mar 20, 2014
Adjust Cart\ItemResolverInterface to remove dependency on Symfony Request class
@pjedrzejewski pjedrzejewski merged commit ec830b0 into Sylius:master Mar 20, 2014
@pjedrzejewski
Copy link
Member

Thanks! Cart component can be updated to reflect this now.

@stloyd stloyd deleted the feature/cart_resolver branch March 20, 2014 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants