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

[Resource] Fix ObjectToIdentifierTransformer bad implementation #3406

Merged

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #2979
License MIT
Doc PR

While fixing #2979 I realized, that ObjectToIdentifierTransformer has inversely implemented methods! It caused mentioned bug, but also some others. There is open PR for this issue (#3085) with implemented custom transformer, but I think it will be not necessary after accepting this one ;)

@michalmarcinkowski
Copy link
Contributor

@Zales0123 what about Behat scenario for this bug?

@Zales0123
Copy link
Member Author

@michalmarcinkowski I can of course write scenario like this:

Scenario: Returning to shipping method page
    Given I go to the checkout start page
      And I fill in the shipping address to United States
      And I press "Continue"
     When I select the "FedEx" radio button
      And I press "Continue"
      And I click "Back"
     Then I should see "FedEx"

but it looks quite artificial. Do we need this only because some bug with some class has occurred? I'm really asking, not judging, sincerely :)

@michalmarcinkowski
Copy link
Contributor

@michalmarcinkowski
Copy link
Contributor

@Zales0123 answering to your question. I think we need a debate about the economy of tests and what we should test using Behat. The test suites are really big right now and I'm not sure what to do with them. On one hand we need tests for such basic, but crucial operations like moving back and forth on the checkout on the other hand we can't test every possible UI interaction with Behat, because it will lead to tests execution for days instead of minutes/hours 😄

@Zales0123
Copy link
Member Author

It's more than satisfying answer 👍 Ok, so for now, I'll add this scenario, to be 100% sure that this nasty bug will not happen anymore.
If it's about deleting ObjectToIdentifierTransformer from SettingsBundle, I'm not certain. Yes, it provides identical logic as this fixed transformer from ResourceBundle, but it is not used as form transformer (notice, it's not even in Form-around namespace), implements specific ParameterTransformerInterface, which is used by SettingsBuilder and is generally totally different type of object.

@michalmarcinkowski michalmarcinkowski added this to the v0.16.0 milestone Oct 18, 2015
pjedrzejewski pushed a commit that referenced this pull request Oct 19, 2015
…sformer

[Resource] Fix ObjectToIdentifierTransformer bad implementation
@pjedrzejewski pjedrzejewski merged commit 937bc6e into Sylius:master Oct 19, 2015
@pjedrzejewski
Copy link
Member

Thank you Mateusz! 👍

@Zales0123 Zales0123 deleted the fix-object-to-identifier-transformer branch October 28, 2016 13:48
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…r-transformer

[Resource] Fix ObjectToIdentifierTransformer bad implementation
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…r-transformer

[Resource] Fix ObjectToIdentifierTransformer bad implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants