Empty node titles cause failures when migrating because empty strings are treated as NULL

Attached is the test above with the patch from #2349819: String field type doesn't consider empty string as empty value minus the tests applied in reverse (had conflicts just reverting it). There will be a lot of fails, but EntityFieldTest::testEmptyStringSave() still failed locally for me, so I am not sure if that issue is the direct cause.

I'm tempted to bump this to critical because there are some fields that disallow null and accept empty strings and this causes a fatal exception. Example, node_field_data.title.

Does that happen if you call ->validate() prior to calling ->save()? Or would the validate() flag that is invalid?

I'm not posting the 10M patch, but if you checkout the commit before #2349819: String field type doesn't consider empty string as empty value (8dc9763d2fa83a52a9b150b07a7dbb634e1575b6) and apply empty_string_field.patch, the new test still fails.

Rather, fields seem to treat an empty string as NULL which is not the same thing.

Strictly speaking, of course you're right than in PHP and in databases, empty string and NULL are different things. But stepping back from that, in terms of Entity API (abstracted from database concerns), what is your business logic that requires distinguishing empty string and NULL with respect to $entity->some_text_field->value?

#12 is the right question : semantically, differentiating between "the field is empty" and "the field has a value, which is empty string" would cause unneeded confusion for 95% use cases of our official "unformatted string" field type.

Especially since the official associated widget (textdield input) can't tell the difference and would turn one into the other on form submission.

As far as thé generic string field type is involved, this is by design.

differentiating between "the field is empty" and "the field has a value, which is empty string" would cause unneeded confusion for 95% use cases of our official "unformatted string" field type.

100% of those times no one would be confused because they would not see the values or the inner workings.

I'm talking about the developpers having to deal with the distinction between "can be empty" and "can be non-empty wit an empty string as value", which in 95% use cases, they don't need and are not aware of.
And also end-users entering values, since, as mentioned above the widget is not able to make the difference and would turn one into the other.

Can I turn it around and ask why I as developer would not be allowed to make that distinction?

Well, as a developper you're free to define your own field type that inherits from StringItem and overrides its isEmpty() method so that it makes the distinction (and accompany it with a widget that lets you input both "empty string" and "no value" and tells them apart). I still think this is a very edge case and does not belong to the generic field type for "unformatted string".

- All of core string fields work that way
- The field type has worked that way since CCK 4.7
- Again, the widget doesn't support '' as an actual value

So yeah, I think qualifying that as an edge case is true.

Care to provide your use case ?

Oh, sorry, you meant it's not true that you can define a different field type that treats empty string as a real value ? How so ?

CCK never really claimed to be _the_ database object abstraction so i don't know how I feel about that comparison

Well, all of core string properties have been converted to the 'string' field type, and none of them needed a semantic difference between "no value" and "empty string as a value" (user name had a glitch for user 0, but that's because of SQL considerations, not because it semantically needs to differenciate two "empty" flavors - semantically, auth users have a non-empty name, the anon user has no name).

Not saying that it means the need doesn't exist, but that it's at least an edge case, if a case at all.
Also, a sample use case was asked a couple times above, but that stayed unanswered.

D6 and D7 allow these properties to be empty strings at the code level so migrations will have to implement weird conversion logic now that they are fields.
[...]
On a personal note, I desperately dislike null as a value in the database. It often leads to problem

That's the thing though - the semantics for fields is : when the field has no value, $entity->field->value is NULL. That's how "empty" behaves for all fields of all field types.
So by having moved to being fields in D8, those properties now follow those common semantics, and will possibly have to convert some associated logic, yes.

Also to consider :
- Views "field is empty" / "field is not empty" filter, relies on NULL in the storage. Two flavors of empty adds confusion there.
- The notion of "required" field : if an empty string is a valid value, then submitting the field with an empty string passes validation ? "required" means empty string is ok ?

"Do I prefer to manipulate NULL or empty string when semantically I mean no value" is about programming style preference, and, true, the move to a unfified Field API kind of robbed you of your freedom there. "No value" is NULL. The good thing is that this is unified for all fields of all types.
"Does my field require both 'no value' and 'empty string as valid value' to be two different things" is about data model semantics. The underlying question is "what does 'required' mean" - is an empty string OK for a required field ? That's not the semantics of the existing core cases, and I'm guessing most of contrib.

Again :
- I don't think we want our mainstream 'string' type (the one used for any text that doesn't use input formats) to have two flavors of "empty" (no value, or a value that is an empty string), that's just confusing for no semantic reason in at least 95% use cases (where empty is just empty).
- if for some reason some specific use case requires a semantic difference between "empty", and "not empty, but with an empty string", then this case can always use a dedicated field type that handles empty string as a non-empty value. Just, all other uses of the 'string' field type don't need the confusion it adds.

Note that the input widget for a field with such a behavior would be something like :
a textfield (that will submit an empty string if empty) + a checkbox for "consider this as 'no value' rather than 'value is empty string'"
I don't think I have ever seen that in an actual form on the actual web, so yes, I think the semantic need is fairly edge case.

Sigh. *THAT* issue.

I will rant some on the background and then we can try to find a solution.

Codd's seminal work establishing relational databases introduced NULL as a representation for "missing information and inapplicable information". This, of course, led to a three value logical applied which well confused everyone. So when a little later people begin to prod and say "you know, missing information and N/A is really not the same" and quietly suggested a four value logic would be even better they were tarred, feathered, railroaded out of town, quartered, hung and burned just to be sure. In other words, this was not a popular suggestion. (And yes, there were arguments for five as well but those works can only be found in the black library of the Vatican and I will deny even mentioning them.)

One of the first relational database implementations was done by a small firm who didn't care that much for seminal works rather just wanted money (the dude didn't change attitudes in the last three decades) so in 1979 Oracle release 2 have stored empty strings and NULLs the same internally and didn't change since. (There was no release 1, ever. Logical release numbers are for wimps, not Larry Ellison.)

Unimaginable chaos followed. Untold number of developers think that NULLs and empty strings are the same because that's the only thing their database provided to them.

A little later HTML was invented when forms were added to HTML, it was an incredible throwback to the darkest ages of computing history with no data types, no semantics. You got key-value string pairs and that was it. Untold number of developers think that NULLs and empty strings are the same because they never really saw a NULL as an input.

On the top of this mess comes Drupal which only in Drupal 7 began to pivot, very slowly, from a form driven system (yes, that typeless mess but then PHP doesn't have types either so wheee!) to an API driven one.

Drupal 8 pretty much finished this pivot and put a very strong emphasis on typed data.

So we are not merely trying to put a square peg in a round hole, no, we are trying to undo either 20 years of HTML/PHP madness or 30 of Oracle rampage, pick your poision. Anyways, it's been going on for a while.

The solution to me (obviously?) then is to lean on, heavily, the typed data system as there's nothing else to lean on. You can not pull a three value answer out of a function which has a binary return value. We are good but noone is that good. Boole and Łukasiewicz are laughing at you just for trying.

So one thing is clear to me: we need isEmpty return TRUE FALSE or NULL and act properly based on some field definition properties. I am drawing a blank on how exactly to continue but of these I am sure.

So the entity system removes empty items in dedicated tables before storage by calling FieldItemList::filterEmptyItems from SqlContentEntityStorage::saveToDedicatedTables

The entity system sets every non-key database column to NOT NULL FALSE in SqlContentEntityStorageSchema::getSharedTableFieldSchema by doing $schema['fields'][$schema_field_name]['not null'] = in_array($field_name, $not_null_keys); . I presume this is done because you might very well have say an image where alt and whatnot are missing and as such are NULLs (and yet the item is not empty because there's a fid).

This is unnecessary and honestly, incorrect for string items as they can not be NULL in HEAD. In fact, most single column fields will claim emptiness if their value is NULL.

So

1. Besides isEmpty we add isNull.
2. add storeNull to field definitions. If this is set to FALSE and single column then we set the column to NOT NULL TRUE. Most of the time this wants to be FALSE but for BC reasons the base class should get a TRUE return and add this one by one to make sure we do not accidentally create a schema for a weird field which on NULL values returned isEmpty FALSE.
3. add storeEmpty to field definitions. If this is set to TRUE then according to storeNull we either do not filter at all or filter just NULLs. What if this is set to FALSE and storeNull is set to TRUE? We need to make a decision of honoring which one or throwing an exception, not a biggie.

This way we have a DB schema that doesn't lie and we can store empty things but not NULLs as larowlan asked in this issue (he wants storeEmpty TRUE, storeNull FALSE).

I think.

Retitling to what I understand the actual bug to be. In itself, how we decide to treat API values is an API decision, but if that also causes or contributes to migration failures, then we should fix it somehow (not necessarily by changing the entity API).

Couple questions:

1. Is it always the case that migrating a node with an empty title from D6 or D7 causes failures? Does this still occur? Migrate has come a long way in the past year, so something else might have already fixed the problem in another way.
2. @yched described a workaround for a particular field definition, but is there a workaround for migrating nodes with empty titles?

As it happens, I closed #2550033: Handle empty fields gracefully today as #2745797: Add option to content entity destinations for validation should provide migrators the option of validating entities before saving.

FWIW, #27 was chx at his best...

there were arguments for five as well but those works can only be found in the black library of the Vatican and I will deny even mentioning them

Working on triaging/reproducing this issue in Drupal 8.4.x with @Stela Oroz

Still happening:

SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'title' cannot be null: INSERT[error]
INTO {node_field_data} (nid, vid, type, langcode, status, title, uid, created, changed,
promote, sticky, revision_translation_affected, default_langcode) VALUES
(:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2,
:db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5,
:db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8,
:db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11,
:db_insert_placeholder_12); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => 1
    [:db_insert_placeholder_2] => article
    [:db_insert_placeholder_3] => und
    [:db_insert_placeholder_4] => 1
    [:db_insert_placeholder_5] =>
    [:db_insert_placeholder_6] => 1
    [:db_insert_placeholder_7] => 1506675097
    [:db_insert_placeholder_8] => 1506675097
    [:db_insert_placeholder_9] => 1
    [:db_insert_placeholder_10] => 0
    [:db_insert_placeholder_11] => 1
    [:db_insert_placeholder_12] => 1
)
 (/Users/kevin/vhosts/fillpdf-service/d7d8/docroot/quick-drupal-20170929084736/drupal/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:777)
Upgrading d7_node:page
Upgrading d7_comment
Upgrading d7_comment_field
Upgrading d7_comment_field_instance
Upgrading d7_comment_entity_display
Upgrading d7_comment_entity_form_display
Upgrading d7_comment_entity_form_display_subject
array_key_exists(): The first argument should be either a string or an integer             [warning]
NestedArray.php:72
array_key_exists(): The first argument should be either a string or an integer             [warning]
NestedArray.php:72

Need to retest on 8.5.x.

Testing in 8.5.x, commit b4ce4af60cc7ffe2c9c8c79a5e2e6d0415a3ef31

Still happening. There is no latest patch, but the latest comments seem to imply that this is an edge case that implementers should handle themselves, and Migrate provides a way for them to do this at the validation step.

Closing as "works as designed."