Catalog promotion channel pricing relation

[Rafikooo]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
License	MIT

[Rafikooo]

I left ->toArray on purpose although foreach below can handle array collection objects.
On line 42 I get appliedPromotions and store them in a variable but in the second step, I clear promotions stored in channelPricing object. After that, it occurs that removing catalog promotions inside channelPricing object also affect local variable $catalogPromotions and clear it also. Alternatively I could use instead of ->toArray clone function which can be more explicit in the future, that we're dealing with a pointer

[Rafikooo]

I could write a spec to assure the right behavior

[lchrusciel]

I'm ok with this implementation, but if the catalog promotion was deleted this by accident, we should check doctrine mapping, to not delete something by accident. I left a few comments related to cascading on mapping and migration. Can you apply them and check if the error still exists?

[Rafikooo]

I resolved mapping-related stuff. I'm not sure if we are on the same page with my comment above, because the problem happens in the application state rather than DB.

A moment ago I discovered that I was wrong. Indeed, there is that pointer/reference case, but it is in src/Sylius/Bundle/CoreBundle/Processor/CatalogPromotionClearer.php:42. After getting ArrayCollection, storing it in $appliedPromotions and callingclearChannelPricing, foreach is skipped. ->toArray produces deep copy so foreach is executed correctly

[Rafikooo]

Added two notes about the code

[AdamKasp]

I think you should squash part of your commits (30 commits IMO is too many)

[lchrusciel]

I'm ok with this implementation, but if the catalog promotion was deleted this by accident, we should check doctrine mapping, to not delete something by accident. I left a few comments related to cascading on mapping and migration. Can you apply them and check if the error still exists?

[Rafikooo]

Checked if I applied all review changes

[lchrusciel]

really minor thing

Suggested change

	foreach ($this->appliedPromotions as $appliedPromotion) {
	if($appliedPromotion->isExclusive()) {
	return true;
	}
	}
	return false;
	return $this->appliedPromotions->exists(function($key, CatalogPromotionInterface $value): bool {
	return $value->isExclusive();
	});

or something like this

Suggested change

	foreach ($this->appliedPromotions as $appliedPromotion) {
	if($appliedPromotion->isExclusive()) {
	return true;
	}
	}
	return false;
	return $this->appliedPromotions->exists(fn($key, CatalogPromotionInterface $value) => $value->isExclusive());

[GSadee]

Suggested change

	* @var ArrayCollection
	* @psalm-var ArrayCollection<array-key, CatalogPromotionInterface>
	*/
	protected $appliedPromotions;
	* @psalm-var ArrayCollection<array-key, CatalogPromotionInterface>
	*/
	protected Collection $appliedPromotions;

[GSadee]

Thank you, Rafał! 🥇