Allow blank value for mobile phone

[tom-combet]

Questions	Answers
Branch?	develop
Description?	Allow blank value for mobile phone in customer address
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes #29379.
Related PRs	N/A
How to test?	Edit customer address with blank mobile phone
Possible impacts?	None

[FabienPapet]

IIRC when you do that, you are setting empty values in database instead of values. Empty phone numbers should IMO be saved as null.

I think the fix should focus constraint instead of editing submitted data. You can play with validation groups to allow null in bo, and disallow in front office.

[tom-combet]

I think the fix should focus constraint instead of editing submitted data.

Which constraint do you make reference to?
I don't think it's about constraints here, as null values are allowed in any case (required is equal to false). It's more about saving null values in the database indeed, but it would be a much larger issue since the empty_data property is a lot used in PrestaShop types...

You can play with validation groups to allow null in bo, and disallow in front office.

The issue doesn't concern FO at all, the Symfony types are only used in BO. By the way, why should it be different in FO?

[FabienPapet]

With the empty_data option, you'll replace a null value in database by an empty string if you save your customer and it is not something you want.

Which constraint disallows null values ? If you want to allow null value, you'll need to find that constraint and either edit it or find a way to disable it with validation groups.

The empty_data option is working , but it has so much side effects and cannot be accepted.

[tom-combet]

Which constraint disallows null values ?

It's not a matter of constraints. If you look at the code, the mobile phone is updated only if the new value is not null (see here). Otherwise, it keeps the old value already present in database.
This is why it's happening only with mobile phone, cause it's the only one non-required field which don't define empty_data option.

I was not aware of the empty_data debate though, so maybe it's better to open an other PR to remove completely empty_data everywhere, in all form types?

[kpodemski]

I think we can accept it to fix the bug and think about other solutions later.

It shouldn't be any problem for now, as we have empty values in this field by default...

[FabienPapet]

The problem for me is with the empty_data function , you'll never have null values, and this is a problem for me as when you are submitting a form, this field will always be updated.

It shouldn't be any problem for now, as we have empty values in this field by default...

@kpodemski what do you mean by empty ? '' or null ?

[FabienPapet]

I thinkl the correct fix here is to compare the value of the object, and the value in the command, if they are different, the we can trigger the update. The fix is for me not at the form level, but at the command level: we should allow to set null in the customer with the CQRS command

[tom-combet]

I thinkl the correct fix here is to compare the value of the object, and the value in the command, if they are different, the we can trigger the update. The fix is for me not at the form level, but at the command level: we should allow to set null in the customer with the CQRS command

I think it's a good idea, but maybe in an other PR? I can but doing it only for phone field is weird, isn't it?

[kpodemski]

ping @FabienPapet - can we unblock this PR? It's a nasty bug that is easily solvable. It's not an ideal way of doing things, but it's the same for other fields too.

[kpodemski]

Thanks, @FabienPapet - I appreciate it. It would be great to have a PoC of how you'd like to see it solved the right way.

[djoelleuch]

Hello @tom-combet ,

I've tested with PHP 7.4 / PHP 8.0 ✔️
Automated tests => OK

Untitled_.Jan.13.2023.8_42.AM.webm

It's QA approved ✔️

thank you for your PR 🚀

[kpodemski]

Thank you, @tom-combet !