Model::setNull() - Allow field value unset #569

mvorisek · 2020-04-23T01:00:35Z

Use case: https://github.com/atk4/ui/blob/06fc859685cdf5f9f3d19772f4305e9eac2d99c0/src/FormField/Lookup.php#L435

codecov · 2020-04-23T01:02:47Z

Codecov Report

Merging #569 into develop will increase coverage by 0.47%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             develop     #569      +/-   ##
=============================================
+ Coverage      86.27%   86.75%   +0.47%     
- Complexity      1232     1234       +2     
=============================================
  Files             28       28              
  Lines           2718     2740      +22     
=============================================
+ Hits            2345     2377      +32     
+ Misses           373      363      -10

Impacted Files	Coverage Δ	Complexity Δ
src/Field.php	`78.91% <100.00%> (+0.38%)`	`96.00 <1.00> (+1.00)`
src/Model.php	`91.11% <100.00%> (+0.08%)`	`377.00 <1.00> (+1.00)`
src/Locale.php	`100.00% <0.00%> (ø)`	`2.00% <0.00%> (ø%)`
src/Persistence/Array_.php	`91.76% <0.00%> (+0.09%)`	`65.00% <0.00%> (ø%)`
src/Util/DeepCopy.php	`88.77% <0.00%> (+0.23%)`	`32.00% <0.00%> (ø%)`
src/Join.php	`83.33% <0.00%> (+0.23%)`	`32.00% <0.00%> (ø%)`
src/Persistence/SQL.php	`75.59% <0.00%> (+0.26%)`	`189.00% <0.00%> (ø%)`
src/Reference/HasOne_SQL.php	`87.23% <0.00%> (+0.27%)`	`41.00% <0.00%> (ø%)`
src/Persistence.php	`85.52% <0.00%> (+6.57%)`	`86.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96add4e...a2b6303. Read the comment docs.

mvorisek · 2020-04-23T20:24:51Z

tests/FieldTest.php

+
+        // invalid value for set() - normalization must fail
+        $this->expectException(\atk4\data\Exception::class);
+        $m->set('c', null); // @TODO even "b"/mandatory field should fail!


@DarkSide666 Am I right, that with "b" field / mandatory field this should fail too?

Yes b (mandatory) should also not allow null value, but allows empty string value.
Basically mandatory should be called notNull and required should be called notEmpty, but changing these property names would be big compatibility break and a lot of code to refactor again in actual projects which use atk4/data :(
Also one of these properties (not sure now which one it was) is more like UI specific property. I guess it's required (notEmpty) one, but again, changing that will break a lot of projects.

@DarkSide666 Yes, I always need to look at the phpdoc for this. notNull and notEmpty will be perfect - ok to refactor? This project is still in very early stage. If we want to make this project big, I think we should not wait with these changes.

To the issue - can you propose a PR with fix before this is merged? The mandatory normalization/validation currently does not work.

Yes notNull and notEmpty would be perfect names for these properties, but changing that will break almost every single data model we have in all our projects :(
Can we change them in at least semi-compatible manner? Maybe introduce isNotNull() and isNotEmpty() methods for that and also use both property names just to keep some compatibility.
On the other hand of course it would be much better (cleaner code) to just change these property names everywhere. Also would like to have @romaninsh input on this. Sadly he's quite hard to catch lately.

@DarkSide666 The current design choose is actually a slightly different - required does not allow everything that evalueated with empty() to true, thus even not null, I understand that it may be handy to set one required property, but it is horrible design choice as mandatory can be false even if the field is effectively mandatory.

Rejecting empty values but still allowing null may be perfectly valid, at least in atk4/data.

Wdyt and what is your prefference? Two bool properties notNull and notEmpty ("empty" excl. null) or one enum property for all 4 possibilities?

I do not see 4 possibilities here - only two.

only restrict null values - everything else (empty string, zero, false etc.) is allowed

restrict null and empty values - do not allow empty string, false, (not sure about this - i think zero should be allowed - it's legit value) zero, empty array etc.

Also there was idea that actually one of these properties more belong to UI than Data level, but it would be quite painful to move it somewhere else because of backward compatibility...

@DarkSide666 - no restriction, prevent null, prevent empty but allow null, prevent null and empty

prevent empty but allow null - I can't imagine of any use-case for this.

@DarkSide666 In data it is very common - null has meaning of N/A, not set yet, while empty means empty value is desired - and not always empty value should be allowed.

DarkSide666 · 2020-04-24T10:53:31Z

tests/FieldTest.php

+
+        // invalid value for set() - normalization must fail
+        $this->expectException(\atk4\data\Exception::class);
+        $m->set('c', null); // @TODO even "b"/mandatory field should fail!


Yes b (mandatory) should also not allow null value, but allows empty string value.
Basically mandatory should be called notNull and required should be called notEmpty, but changing these property names would be big compatibility break and a lot of code to refactor again in actual projects which use atk4/data :(
Also one of these properties (not sure now which one it was) is more like UI specific property. I guess it's required (notEmpty) one, but again, changing that will break a lot of projects.

DarkSide666 · 2020-04-24T10:58:20Z

Please add:

        $this->expectException(\atk4\data\Exception::class);
        $m->set('b', null);

Besides that setNull method looks fine to me - like a bit of workaround, but fine.
Important is to not allow to set null value in normal way with set. But if developer uses setNull then he should know what exactly he is doing - it can't happen accidentally, so it's fine then.

mvorisek · 2020-04-24T11:21:01Z

Please add:
        $this->expectException(\atk4\data\Exception::class);
        $m->set('b', null);
Besides that setNull method looks fine to me - like a bit of workaround, but fine.
Important is to not allow to set null value in normal way with set. But if developer uses setNull then he should know what exactly he is doing - it can't happen accidentally, so it's fine then.

See above, I want (and actually did), but the current implementation is broken...

mvorisek · 2020-04-24T11:23:09Z

@DarkSide666 As this PR is complete, you can merge it and solve the mandatory bug later and change then the 924 line in tests to use b field.

DarkSide666 · 2020-04-24T11:29:14Z

@DarkSide666 As this PR is complete, you can merge it and solve the mandatory bug later and change then the 924 line in tests to use b field.

OK I agree.

mvorisek force-pushed the set_null branch from 09ced6b to c15a05d Compare April 23, 2020 01:02

Allow field value unset (set to null by Model::setNull())

58a6b47

mvorisek force-pushed the set_null branch from c15a05d to 58a6b47 Compare April 23, 2020 01:16

mvorisek changed the title ~~Allow field value unset (set to null by Model::setNull())~~ Model::setNull() - Allow field value unset Apr 23, 2020

mvorisek closed this Apr 23, 2020

mvorisek reopened this Apr 23, 2020

mvorisek force-pushed the set_null branch 3 times, most recently from a34afab to 069d01d Compare April 23, 2020 01:32

mvorisek requested review from DarkSide666 and PhilippGrashoff April 23, 2020 01:33

wip

9580940

mvorisek force-pushed the set_null branch 2 times, most recently from b35eaf1 to 68b5ae7 Compare April 23, 2020 20:21

mvorisek commented Apr 23, 2020

View reviewed changes

mvorisek requested review from abbadon1334 and georgehristov April 23, 2020 20:26

mvorisek marked this pull request as ready for review April 23, 2020 20:26

mvorisek force-pushed the set_null branch 2 times, most recently from 2399d96 to 155fe0c Compare April 23, 2020 20:30

mvorisek added the in review label Apr 23, 2020

implement

a2b6303

mvorisek force-pushed the set_null branch from 1c44e0d to a2b6303 Compare April 23, 2020 20:34

DarkSide666 requested changes Apr 24, 2020

View reviewed changes

DarkSide666 self-requested a review April 24, 2020 11:29

DarkSide666 approved these changes Apr 24, 2020

View reviewed changes

DarkSide666 merged commit 1c5c361 into develop Apr 24, 2020

DarkSide666 deleted the set_null branch April 24, 2020 11:30

mvorisek mentioned this pull request Apr 25, 2020

"mandatory" and "required" field properties need to be refactored #576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model::setNull() - Allow field value unset #569

Model::setNull() - Allow field value unset #569

mvorisek commented Apr 23, 2020 •

edited

Loading

codecov bot commented Apr 23, 2020 •

edited

Loading

mvorisek Apr 23, 2020

DarkSide666 Apr 24, 2020

mvorisek Apr 24, 2020

DarkSide666 Apr 24, 2020

mvorisek Apr 24, 2020

DarkSide666 Apr 24, 2020

mvorisek Apr 24, 2020

DarkSide666 Apr 25, 2020

mvorisek Apr 25, 2020

DarkSide666 Apr 24, 2020

DarkSide666 commented Apr 24, 2020

mvorisek commented Apr 24, 2020

mvorisek commented Apr 24, 2020

DarkSide666 commented Apr 24, 2020

Model::setNull() - Allow field value unset #569

Model::setNull() - Allow field value unset #569

Conversation

mvorisek commented Apr 23, 2020 • edited Loading

codecov bot commented Apr 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarkSide666 commented Apr 24, 2020

mvorisek commented Apr 24, 2020

mvorisek commented Apr 24, 2020

DarkSide666 commented Apr 24, 2020

mvorisek commented Apr 23, 2020 •

edited

Loading

codecov bot commented Apr 23, 2020 •

edited

Loading