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

[PromotionBundle] The collection form type uses the new CollectionExtension #2239

Merged
merged 3 commits into from
Dec 16, 2014

Conversation

arnolanglade
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Behat? no
Phpspec? yes
Fixed tickets -
License MIT
Doc PR -

@arnolanglade arnolanglade added PromotionBundle Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). labels Dec 9, 2014
->add('type', 'sylius_promotion_rule_choice', array(
'label' => 'sylius.form.rule.type'
))
->addEventSubscriber(new BuildRuleFormListener($this->checkerRegistry, $builder->getFormFactory()), $options['rule_type'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrongly placed ). Should be:

- ->addEventSubscriber(new BuildRuleFormListener($this->checkerRegistry, $builder->getFormFactory()), $options['rule_type'])
+ ->addEventSubscriber(new BuildRuleFormListener($this->checkerRegistry, $builder->getFormFactory(), $options['rule_type']))

@arnolanglade arnolanglade force-pushed the promotion branch 5 times, most recently from b58acd2 to 7b06c01 Compare December 11, 2014 21:13
@arnolanglade
Copy link
Contributor Author

@pjedrzejewski I need to fix spec and behat but what do you think about it?


/**
* Shipping country rule configuration form type.
*
* @author Saša Stamenković <umpirsky@gmail.com>
*/
class ShippingCountryConfigurationType extends AbstractResourceType
class ShippingCountryConfigurationType extends AbstractType
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of 'class' => $this->dataClass. class is not data_class.

@pjedrzejewski
Copy link
Member

Looks really good to me, nice work Arnaud! 👍

@arnolanglade arnolanglade force-pushed the promotion branch 3 times, most recently from 3461d16 to 8e0a5f6 Compare December 12, 2014 23:21
@arnolanglade
Copy link
Contributor Author

@Sylius/core-team / @stloyd It is time to code review !

@stloyd
Copy link
Contributor

stloyd commented Dec 13, 2014

@Arn0d Looks good for me, nice job ;)

@arnolanglade
Copy link
Contributor Author

You guys, I get exactly the same errors than the country form (I speak about behat). I run them manually and it works! @pjedrzejewski what do you think ?

@arnolanglade
Copy link
Contributor Author

ping @pjedrzejewski

@arnolanglade arnolanglade changed the title [WIP][PromotionBundle] The collection form type uses the new CollectionExtension [PromotionBundle] The collection form type uses the new CollectionExtension Dec 15, 2014
pjedrzejewski pushed a commit that referenced this pull request Dec 16, 2014
[PromotionBundle] The collection form type uses the new CollectionExtension
@pjedrzejewski pjedrzejewski merged commit 983bab1 into Sylius:master Dec 16, 2014
@pjedrzejewski
Copy link
Member

Thank you Arnaud! Really nice work! 👍

@arnolanglade
Copy link
Contributor Author

✌️

@arnolanglade arnolanglade deleted the promotion branch December 16, 2014 12:06
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

3 participants