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

[Promotion] Clean up promotions handling #6000

Merged
merged 12 commits into from
Sep 9, 2016

Conversation

pamil
Copy link
Contributor

@pamil pamil commented Sep 7, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Related tickets -
License MIT

@pamil pamil force-pushed the promotion-processor-2 branch 3 times, most recently from b0ec531 to dc05ac4 Compare September 7, 2016 11:13
@pjedrzejewski pjedrzejewski added BC Break PRs introducing BC breaks (do not even try to merge). Bug Fix labels Sep 7, 2016
@@ -196,7 +196,7 @@ Below you can see how it works:
* @param ServiceRegistryInterface $registry
* @param EventDispatcherInterface $dispatcher
*/
$checker = new PromotionEligibilityChecker($checkerRegistry, $dispatcher);
$checker = new CompositePromotionEligibilityChecker($checkerRegistry, $dispatcher);
Copy link
Member

Choose a reason for hiding this comment

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

$checker = new CompositePromotionEligibilityChecker($checkerRegistry);

@michalmarcinkowski
Copy link
Contributor

Thanks Kamil! 👍 Please apply comments in a separate PR.

@michalmarcinkowski michalmarcinkowski merged commit a567b3b into Sylius:master Sep 9, 2016
@pamil pamil deleted the promotion-processor-2 branch October 5, 2016 15:07
@rvanlaak
Copy link
Contributor

rvanlaak commented Oct 18, 2016

generateUniqueCode visibility changed from public > private, which is a BC break.

While I'm okay with making BC breaks as long as beta1 isn't released yet this wasn't documented in UPGRADE.md.

Please document the new way of creating promotion codes by using a InstructionInterface.

@pamil
Copy link
Contributor Author

pamil commented Oct 18, 2016

generateUniqueCode was in fact changed from protected to private (but that's still BC).

You can still use any PromotionCouponGeneratorInterface instance to generate coupons, the signature is as follows:

/**
 * @param PromotionInterface $promotion
 * @param PromotionCouponGeneratorInstructionInterface $instruction
 *
 * @return array of generated coupons with coupon code as a key
 */
public function generate(PromotionInterface $promotion, PromotionCouponGeneratorInstructionInterface $instruction);

@rvanlaak
Copy link
Contributor

Oh wow, I came from 0.17 yesterday so before then it still was public: pamil@4c9c6a0#diff-b933567465782f701b9cadb26f36d5d7L77

Still, it would be nice if such changes are documented, but I think right now too many BC breaks are made to document all of them right? Are there any new docs on how to create coupons? What's Sylius's point of view on this, because I don't want to write libraries on all the Sylius classes I've integrated ;-)

@pamil
Copy link
Contributor Author

pamil commented Oct 18, 2016

0.17 feels like it was a lifetime ago :)

There were so many BC breaks introduced recently (classes made final; removing unused bundles, components and single classes), we got brand new frontend etc., so documenting them was quite tremendous task. I guess that we will improve it since we reach 1.0.0-alpha (which is really, really soon). Nevertheless, if you rely on interfaces to customise the application behaviour, the BC breaks shouldn't be so crucial :)

@TheMadeleine keeps up with extremely good work documenting Sylius so there indeed is a documentation about generating coupons.

@rvanlaak
Copy link
Contributor

Ghehe yes 0.17 actually was ages ago ;) I'm only using the Promotion bundle for now so the errors I get quite often are related to missing classes :)

I'm kinda used to follow upgrade logs, but didn't see any mentions about the changes to the Promotion component. Actually I did try upgrading a couple of months before, which didn't work out then because I had to do too much maintenance work for it. Over the last days I actually also did take a look in the commits made to the Promotion component, but somehow missed this one. So it would be awesome to see a docs hackday ghehe ;)

I'll do a hotfix now, good luck finishing 1.0.0-alpha in the next few days guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break PRs introducing BC breaks (do not even try to merge).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants