Fix empty text transformer to not treat zero as empty text #9120
Conversation
|
Look at the class name |
|
it is TextEmptyType class, I don't understand lol |
|
The problem is that there are some text types that must allow zero as a value. One good example is in shipping->preferences page the "free shipping starts at" field. It must allow saving 0 but it doesn't because 0 is transformed i to an empty string and empty string is not a valid value. |
|
@rokaszygmantas I'm already convinced because I'm aware that |
|
True that. But do you see the described situation as a bug or do you think it's intended behaviour? :) |
|
What really happened if the value is empty (0, or ''), is that break something with your template, controller or something? If it's break something, maybe we need to change the field type by a NumberType ? |
|
Yes, in my case the field is in shipping > preferences page, called "free shipping starts at" and it allows to enter weight. Number type would fit here but it needs to show weight unit next to the field, so I used TextWithUnitType for it. |
|
I think the problem can be better seen with these fields:
|
|
How about: // empty(0) returns true...
return (empty($data) && is_null($data)) ? '' : $data;We keep the "right" condition and fix the bug that comes from PHP 💪 |
|
@mickaelandrieu Your test is wrong, no need to do an empty if you use is_null :/ |
|
@PierreRambaud please add your test before saying mine is wrong :)
Note that I keep 'empty()' for explicit the "intent" of the test, of course we could remove it 👍 |
So if we can remove, the original PR is good, we can just do |
We can but I don't think we "should". We are fixing a bug from PHP here, but our intent is to allow input type with "empty" text, not "nullable" one. Being empty and being null is not the same! |
|
I don't understand where the PHP bug is, it's normal behavior for |
|
We disagree on this: If we change the test from "empty" to "null" we must the class name so we must create another FormType named |
|
But it's clearly weird and useless to have an empty and |
I'm not against using ===, I'm against using a class name not related to what we are doing in it: it's not SOLID and we should stop doing that cause of bugs of PHP language :)
it's not! It describes the intent of the class: it's also useless to have real variables names like $users, we could name our variables $a, $b, $c and $d. For the PHP engine, it's even better but we write code for developers and not for the engine.
Thinking about it, it's not always possible as some fields should accept strings or floats values. In all cases, the form component retrieves string values from client data and then do the casts when needed. In the specific use case of custom values, we can't use NumberType form. Also, how about TextareaEmptyType form? Probably the same bug, even if it's really uncommon to set "0" as value in a textarea input (used for descriptions fields most of the time) To sumup my pov on this patch:
|
So it's a class name problem not a PHP bug. (Because it's not a bug just a stupid feature Otherwise, we have a class which name is Adding a More, and because the is_null test is useful, we can maybe just use the default |
).|
We can't rely on default This weird behavior of Symfony may have been fixed in v3 (empty values are not "posted"), I don't know :) EDIT: bip bip! It has been fixed since Symfony 3.1 => symfony/symfony#5906 let's use original {Text|TextArea}Type instead, and deprecate my owns 🎉 @rokaszygmantas what you need to do:
We can't remove these classes right now, to keep the compatibility. |
a202c6b to
0457e0f
Compare
|
after removing the empty text types service declarations, use case from this pr is no longer working: #5822 - I cannot save empty value for stock label 😕. I'll debug it a bit more tomorrow 🙂 |
|
@rokaszygmantas shouldn't use the default label as default value? Maybe it's a side effect of a patch of Symfony => https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/TextType.php#L28 |
|
@rokaszygmantas let me know when you have time for this, I'd like to see this pull request merged by the end of the week 👍 |
|
@mickaelandrieu sorry for delaying this PR. I'll try to tackle it ASAP. I've investigated the issue with stock label a bit more and found out there are two problems actually:
It looks like those two bugs now are coming from legacy code, wdyt @mickaelandrieu? 🪲 🪲 |
|
@rokaszygmantas thanks for the information, really interesting and looks like an issue not related to the migration of your page at all. What I think now: we need to address these 2 bugs but I want you to stay focused on migration, so we will find someone else to work on this /c @eternoendless Regarding the page you have migrated, I think will should merge it with this known issue in mind. |
3cf6988 to
72074ec
Compare
| @@ -59,7 +59,8 @@ public function __construct( | |||
| array $locales, | |||
| CurrencyDataProvider $currencyDataProvider, | |||
| array $tosCmsChoices | |||
| ) { | |||
| ) | |||
There was a problem hiding this comment.
it's weird, not in psr2 or symfony rules :/
There was a problem hiding this comment.
Which rule do you use to have this?
| $form->add('file', 'Symfony\Component\Form\Extension\Core\Type\FileType', array('mapped' => false)); | ||
| $form->add('name', 'Symfony\Component\Form\Extension\Core\Type\TextType', array('mapped' => false)); | ||
| $form->add('file', FileType::class, ['mapped' => false]); | ||
| $form->add('name', FileType::class, ['mapped' => false]); |
There was a problem hiding this comment.
Name must be a text type
| [], | ||
| 'Admin.Catalog.Feature' | ||
| ) . ' ('; | ||
| 'Use default behavior', |
There was a problem hiding this comment.
weird indent, which rule do you used?
|
@mickaelandrieu What rules do you used for cs fixer, some aren't in PSR2, PSR1, and not even in Symfony rules? |
|
@PierreRambaud I'll check my PHPStorm rules, but nothing I have personally set up |
72074ec to
0274473
Compare
|
@PierreRambaud I've fixed the wrong form type and used PHP CS Fixer on the folder: I'm not sure this change the weird indentation you noticed :/ |
0274473 to
518ff2f
Compare
|
What do you mean by |
| @@ -33,17 +34,17 @@ class TinyMceMaxLengthValidator extends ConstraintValidator | |||
| { | |||
| public function validate($value, Constraint $constraint) | |||
| { | |||
| $replaceArray = array( | |||
| $replaceArray = [ | |||
| "\n", | |||
| "\r", | |||
| "\n\r", | |||
There was a problem hiding this comment.
I just like how this line is useless when the first and second one can do the job --'
More I found a potential problem in the future here:
$str = "Line 1\nLine 2\rLine 3\r\nLine 4\n";
$order = array("\n", "\r");
$replace = '<br />';
$newstr = str_replace($order, $replace, $str);
var_dump($newstr);
// Will produce
/*
$ php test.php
/home/got/test.php:10:
string(54) "Line 1<br />Line 2<br />Line 3<br /><br />Line 4<br />"
*/When
$newstr = str_replace($order, [$replace], $str);
// Will produce
/*
$ php test.php
/home/got/test.php:10:
string(42) "Line 1<br />Line 2Line 3<br />Line 4<br />"*/As you can see, it is replaced by null and not an empty string (because index isn't found).
Currently null = '' but this isn't what we expect.
There was a problem hiding this comment.
we agree I shouldn't change this behavior in this pull request, am I right?
There was a problem hiding this comment.
I think you can, otherwise we'll never fixed it! (or not now at least)
Leave your code better than you found it. (https://deviq.com/boy-scout-rule/ in Agile XP)
There was a problem hiding this comment.
Don't you think it's already far better? My fix concerns only 10 lines of this pull request, I've improved SIXTY-EIGHT files.
More, I don't want to change the behavior and risk a regression on it.
|
Ping @prasanthSelvar It fixes the issue you pointed in #9094, would you mind to validate it? |
|
wdyt @PierreRambaud? |
|
Thanks @mickaelandrieu |
This change is