NotNull validation constraint does not fail on empty strings [#2444585]

Problem/Motivation

Creating nodes from REST I have detected that I can create new nodes with empty title.

TypedData\Map's implementation of ::isEmpty() does strict type checking, so "" is not considered an empty value and hence passes the NotNull validation.

However, user module implicitly relies on this behaviour as during user_install it creates a user with no name (UID 0). So this means fixing it to treat "" as empty breaks in spectacular fashion.

Because our validation API does not in fact validate required fields, this could lead to data integrity issues and could result in some fairly hairy update hooks to repair broken stuff.

There are other issues in the queue around 'unable to remove empty text fields', probably the same issue as this. E.g. #2349819: String field type doesn't consider empty string as empty value

Proposed resolutions

Two possible fixes:

Sub-class StringItem for username (UsernameItem) and don't consider "" empty in that case. Change Map to consider "" as empty (as well as NULL). Change User to use UsernameItem. The existing UserName constraint will validate users (other than the one created in user_install()). -OR-
Force anonymous user to have a username - e.g. 'anonymous' and make this a reserved username (See #18)

Comment	File	Size	Author
#18	notnull-constraint-2444585.16.patch	4.83 KB	larowlan
#18	8.0.x: PHP 5.5 & MySQL 5.5 10,282 pass, 483 fail
#18	interdiff.txt	3.01 KB	larowlan
#15	notnull-constraint-2444585.15.patch	1.82 KB	larowlan
#15	8.0.x: PHP 5.5 & MySQL 5.5 10,374 pass, 819 fail
#15	interdiff.txt	1.14 KB	larowlan
#13	notnull-constraint-2444585.2.patch	1.5 KB	larowlan
#13	8.0.x: PHP 5.5 & MySQL 5.5 13,201 pass, 14 fail
#7	2444585-6.patch	1.43 KB	marthinal
#7	8.0.x: PHP 5.5 & MySQL 5.5 10,370 pass, 820 fail
#7	field_validation-only-2444585-6-test.patch	853 bytes	marthinal
#7	8.0.x: PHP 5.5 & MySQL 5.5 13,212 pass, 2 fail
	Screen Shot 2015-03-02 at 12.05.52.png	16.94 KB	marthinal
	field_validation-only-test.patch	853 bytes	marthinal

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

2 March 2015 at 20:52

Status:

Needs review

» Needs work

The last submitted patch, field_validation-only-test.patch, failed testing.

Comment #2

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 3 March 2015 at 09:19

Issue tags:

+Entity Field API

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir commented 9 March 2015 at 01:49

Title:	Field validation does not work as expected.	» NotNull validation constraint does not fail on empty strings
Assigned:	Unassigned	» fago

Better title :)

This is really bad, I don't know why we didn't figure this out earlier?

NotNullValidator really is a !== NULL check.

I can currently see two ways to fix this:

a) Make it also fail on empty strings (like the UI which depends on form API for this does).

b) Somehow *reliably* filter out empty field items before running the validation.

Comment #4

14 March 2015 at 16:47

prics queued field_validation-only-test.patch for re-testing.

Comment #5

14 March 2015 at 17:05

The last submitted patch, field_validation-only-test.patch, failed testing.

Comment #6

fago

German

Vienna

CreditAttribution: fago commented 19 March 2015 at 11:42

Assigned:

fago

» Unassigned

I think, validation should match the field's isEmpty() logic - so if that fails also this is probably the problem.

NotNullValidator really is a !== NULL check.

Right. I've been already thinking whether it would make sense to provide a custom NotEmpty constraint instead of just relying on symfony ones. Right now, the validator does extra massaging of the values to make empty data structures NULL to make it work, see PropertyContainerMetadata::accept(). So maybe the problem here is just a not good isEmpty() implementations of the field type?

Howsoever, my thought was that NotEmpty constraint could just work with the object and call $typed_data->isEmpty() itself, what should help simplifying the validator logic. Not sure it's related, depending on where the bug really is here.

Comment #7

marthinal CreditAttribution: marthinal commented 10 August 2015 at 17:44

Status:	Needs work	» Needs review
Issue tags:		+REST

File	Size
field_validation-only-2444585-6-test.patch	853 bytes
8.0.x: PHP 5.5 & MySQL 5.5 13,212 pass, 2 fail
2444585-6.patch	1.43 KB
8.0.x: PHP 5.5 & MySQL 5.5 10,370 pass, 820 fail

From Rest I'm updating fields for users. I've detected that the situation is the same. I can update the value of a required field with an empty string and in this way you can remove the data from fields...

I think that this patch fixes the problem, at least for user update and when creating a node. I was testing from the rest client so let's take a look at the results.

Comment #8

10 August 2015 at 18:18

Status:

Needs review

» Needs work

The last submitted patch, 7: 2444585-6.patch, failed testing.

Comment #9

10 August 2015 at 18:39

The last submitted patch, 7: field_validation-only-2444585-6-test.patch, failed testing.

Comment #10

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 August 2015 at 03:44

Priority:	Major	» Critical
Issue tags:		+Needs Drupal 8 critical triage

Discussed with @webchick, given this involves data loss (see last comment) this may in fact need to be critical.

Feel free to drop back.

Comment #11

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 August 2015 at 03:44

Issue tags:

-Needs Drupal 8 critical triage

Removing the tag

Comment #12

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 August 2015 at 03:46

Assigned:

Unassigned

» larowlan

kicking along as per #6

Comment #13

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 August 2015 at 04:40

Status:

Needs work

» Needs review

File	Size
notnull-constraint-2444585.2.patch	1.5 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,201 pass, 14 fail

I think we have a few issues in the queue around not being able to remove string/text fields in multi-value setups.

This might solve those too.

We might need similar treatments for numeric fields.

Comment #14

11 August 2015 at 05:07

Status:

Needs review

» Needs work

The last submitted patch, 13: notnull-constraint-2444585.2.patch, failed testing.

Comment #15

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 August 2015 at 05:25

Status:

Needs work

» Needs review

File	Size
interdiff.txt	1.14 KB
notnull-constraint-2444585.15.patch	1.82 KB
8.0.x: PHP 5.5 & MySQL 5.5 10,374 pass, 819 fail

Or maybe this approach

Comment #16

11 August 2015 at 05:30

Status:

Needs review

» Needs work

The last submitted patch, 15: notnull-constraint-2444585.15.patch, failed testing.

Comment #17

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 August 2015 at 06:08

Issue summary:

View changes

gold, user_install() relies on creating an invalid user, with an empty username, despite the UserName constraint considering this invalid.

Comment #18

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan as a volunteer commented 11 August 2015 at 06:25

Status:

Needs work

» Needs review

File	Size
interdiff.txt	3.01 KB
notnull-constraint-2444585.16.patch	4.83 KB
8.0.x: PHP 5.5 & MySQL 5.5 10,282 pass, 483 fail

Perhaps we can make anonymous a reserved username
Short of that we can make user-name its own item type, we already did that for password.

Comment #19

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting for Drupal Association commented 11 August 2015 at 06:27

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItem.php
@@ -100,4 +100,24 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
+      if (!$definition->isComputed() && $property->getValue() !== NULL && $property->getValue() !== "") {

How can any of the properties of a StringItem be computed? IMHO I just see value in there, so this maybe need some documentation? Let's also use single quotes :P