Remove the remaining soft deleteable code

[coudenysj]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	?
Deprecations?	yes
Related tickets	fixes #4761 and #4461
License	MIT

TODO

• Cleanup Behat tests
• Check if DoctrineSluggablePass can be removed

[michalmarcinkowski]

$entityManager is not needed anymore, as well as comment above.

[coudenysj]

Looks like TravisCI is having a lot of difficulties?

[Zales0123]

@coudenysj yes, sadly it seems to fail with latest chrome version. All should be fixed after merging #4786.

[coudenysj]

As far as I can tell, the soft deleteable magic is not tested in any Behat feature file.

We have to wait on #4786 to see if anything breaks, I guess.

[michalmarcinkowski]

@coudenysj #4786 was merged, please rebase.

[coudenysj]

@michalmarcinkowski Done!

[michalmarcinkowski]

Great job! Thanks Jachim! 👍

[pjedrzejewski]

Awesome work Jachim, thank you so much!

[rvanlaak]

Great! Thanks, the softdeleteable also resulted into errors when the promotion bundle was used stand-alone. Can we let userland just enable it before and disable it after we use CouponGenerator::generateUniqueCode?

[pjedrzejewski]

@rvanlaak I am not sure what you mean? Soft-deleteable behavior is gone from Sylius.

[rvanlaak]

@pjedrzejewski yes I know, but I'm still on 0.17 and the change was merged after that release. So basically my question is whether, until another version is released, I can just enable it before and disable the filter afterwards?

Oh wait, I see 0.18 is released yesterday. Think I'm going to upgrade right away. Major breaks on promotion?

[rvanlaak]

The upgrade to 0.18 wasn't easy, got a missing factory error while compiling the container. Probably has something to do with a missing dependency if promotion is used stand-alone.

[michalmarcinkowski]

@rvanlaak did you resolve this issue?

[rvanlaak]

@michalmarcinkowski no we postponed the upgrade for now, but will try again soon. Did hope that the documentation on the upgrade can be improved, given 0.18 has that many changes related to the promotion component: https://github.com/Sylius/Sylius/blob/master/CHANGELOG.md

[rvanlaak]

We updated from 0.17 to 0.19 today, basically rewriting the softDelete functionality took us quite some time. We are fine with removing it from the Promotion entity, but we ourselves actually have a use-case for it on the Coupon entity. When customers bought a coupon from us but don't want to use it anymore, we should be able to "delete" it.

@michalmarcinkowski are there any plans to simulate the softDelete behaviour for the Coupon entity, but without using the DoctrineExtensions? The only thing about the decision proces I can find on it is you saying that is should be removed in #4761 (comment), but I'm not exactly sure why :)

[coudenysj]

@rvanlaak I think a solution could be to use the ToggleableInterface.

[michalmarcinkowski]

@rvanlaak if coupon is not used you should be able to delete it, if it was already used it should be archived. See #4140 and our idea for archiving.

[michalmarcinkowski]

Btw. for coupons you can simply set "expiresAt" to make it archived :)

[rvanlaak]

Ah thanks for referencing #4140, I agree with the problem statement given there 👍 For now I was able to reuse the deletedAt field that already exists by overriding the Coupon class used by the promotion component:

/**
 * @ORM\Table(name="sylius_promotion_coupon")
 * @ORM\Entity
 *
 * @Gedmo\SoftDeleteable(fieldName="deletedAt")
 */
class Coupon extends BaseCoupon
{
    use SoftDeleteableEntity;
}

After the first alpha is released we will migrate it to expiresAt. Wouldn't it be better to "improve" the ToggleableTrait and use an expiresAt datetime instead of the boolean that is used now? The bool doesn't tell us anything about the coupon's history (read: archive date).

[michalmarcinkowski]

Not sure what do you mean? PromotionCoupon is not toggleable.

[rvanlaak]

Yes that's correct, so I'd like to propose to make PromotionCoupon toggleable and would like to know if you agree on that too.

The second thing I'd like to mention (aside of making it toggleable itself), is that it would be great if ToggleableTrait::$enabled could become a DateTime to provide more info about the lifecycle of the entity. In other words: so we can see when the coupon was archived. That would also make archiving in the future possible:

/**
 * @return Boolean
 */
public function isEnabled()
{
    if (null !== $this->enabled) {
        return $this->enabled <= (new \DateTime());
    }

    return false;
}

[michalmarcinkowski]

That seems redundant. Why not simply use expiresAt for this? The logic is already implemented and working.

[rvanlaak]

Okay thanks 👍 For our use-case the archiving has a different use-case than the expiresAt field would have so I'll stick with my custom implementation for now.