Make quantity and quantity_per_user nullable in cart_rule table

[boherm]

Questions	Answers
Description?	Make quantity and quantity_per_user nullable in cart_rule table. Related to PrestaShop/PrestaShop#40330
Type?	improvement
BC breaks?	no
Deprecations?	no
Fixed ticket?	~
Sponsor company	PrestaShop SA
How to test?	~

[Quetzacoalt91]

Blocked until PrestaShop/PrestaShop#40330 is merged

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[AureRita]

Hi @boherm

Thank you for your PR, I tested it and it seems to works as you can see :

With PR :

without PR :

Tested from :
9.0.2 to 9.1.0
8.1.4 to 9.1.0

Because the PR seems to works as expected, It's QA ✔️

Thank you

[Hlavtox]

BC break

Code may stop working or throw errors if nulls are present:
if ($row['quantity'] > 0) { ... }

SQL queries with WHERE quantity = 0 can fail and not select proper lines.

[Quetzacoalt91]

Thanks for the feedback @Hlavtox, however are you sure this is the proper place for it?

This PR is focused on the change of the database regarding another PR already merged on the core, it will be merged because Update Assistant aims to be aligned with PrestaShop. If there is a BC Break, you should warn about it either on the Core PR and/or an issue on https://github.com/PrestaShop/PrestaShop so it can be dealt with the Core team separately. 🙏

[Hlavtox]

@Quetzacoalt91 I know, but there is no need to merge changes into autoupgrade, when the core PR is wrong and should be changed. :-)

[jolelievre]

@Hlavtox, this is not a breaking change, the column is made nullable, it doesn't mean we force the null value! The default value is still 0, which means all the existing cart rules will work as usual, and all the cart rules created via migration using the legacy page will work as usual.

What will change? It will only change IF you enable the discount feature flag AND you create a new discount AND you select no limit in the discount you are creating. Only in this case will you get a null value in your DB, and that's the purpose of a feature flag and an experimental feature.

The problem you mentioned can also only happen in this case, and it won't happen in the core because we already fixed/adapted the relevant places. So it's only in an external module that you will have this problem. What does it mean then? It means the module needs to adapt its code to be compatible with the future discount feature; it means that it can anticipate this change to be compatible with the future major version that will include this new discount feature.

And that's exactly the purpose of adding an experimental feature behind a feature flag. So your blocking of this PR and the modification in the core is not relevant.

[Hlavtox]

So it's only in an external module that you will have this problem.

Dude, that's the whole purpose of BC break promise.

The problem is that if you enable the new discount feature, create this cart rule with no limit (which is by the way 99% of our cart rules), then you disable it, you still have the "faulty" code that will keep breaking external modules. This is a BC break.

[jolelievre]

The problem is that if you enable the new discount feature, create this cart rule with no limit (which is by the way 99% of our cart rules), then you disable it, you still have the "faulty" code that will keep breaking external modules. This is a BC break.

Ok I anticipated this argument, but my message was already too long 😅 So experimental features are called experimental for a good reason, you're not supposed to test them on a prod environement If you want to test it safely you use a preprod or a local environment, from the moment you enable an experimental feature you are in a development context And we can't guaranty full retro compatibility from this moment even if you revert the FF

Ideally, I agree with it should be the case, but it would mean we need to implement a special revert action for each feature flag and frankly, we don't have enough time or resources for this

[Hlavtox]

Hmmmm, well, OK. But it would be good to establish this "grey zone" in our ADR. :-)

Something like - it's not a BC break if you break your install because of experimental feature. WDYT?

[nicosomb]

@Hlavtox is that ok for you to remove your block on this PR?

[Hlavtox]

I made an issue so it's not forgotten - PrestaShop/ADR#44

[jolelievre]

@Hlavtox thanks, I noted it in my tasks but the issue doesn't hurt