String field type doesn't consider empty string as empty value

Yes string fields should override isEmpty. Most of the other field types do. The isEmpty() from email fields should work here.

Should be fixed by #1988492: Avoid having empty field items by default

Talked to Amateescu, we should probably fix this in StringItemBase

Wrong patch

Whut ?

Wow, let's see what it does on StringItem

Fago suggested filling in Anonymous at install ..

I think SrtingItem::isEmpty() not considering an item with value '' (empty string) to be an empty item, is definitely a bug, and a major one IMO.

As the symptom in the initial bug report shows, this goes against what any widget for the "string" field type would do : a form element without user input, but not considered empty ?

Imagine the field is required :
- leaving the input empty would fail Form API #required validation
- but $entity->validate() would accept empty string as a value, because the the field has an item (with value empty string) ?
WTF :-)

If we fill in a value for anon user name, then we can make the user.name field actually required ?

If we fill in a value for anon user name, then we can make the user.name field actually required ?

Yes, that would make a ton of sense - wouldn't it?

So, this means we can remove the hack in UserStorageSchema right ?

Well, even if the field is required, SqlContentEntityStorageSchema::getSharedTableFieldSchema() would still not mark the column as "not null = TRUE", because of that line :

    // Label fields are not necessarily required.
    unset($not_null_keys['label']);

:-)

As we said yesterday, now that User 'name' field is actually required, we can try a quick check see if we still have cases of entity types with a non-required label, but until then the 'name' column needs that hack in UserStorageSchema to keep its "not null = TRUE" :-/

Hmm yeah, interesting :/
A lot of failures are now due to the title of a node being NULL ...

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'title' cannot be null: INSERT INTO {node_field_data} (nid, vid, type, langcode, title, uid, status, created, 

IIRC, this is now because filterEmptyItems() now also gets into the StringItem and calls isEmpty() right ?

First batch of changes

Let's see if the bringing back the defaultValue on Node triggers less fails

Marked #2478077: Markup (and label) of empty fields get rendered as a duplicate.

Also, better title.

Better better title :)

Not only are empty fields still there after edit, but each revision adds a new empty field item to the storage table.

Here is a "workaround" patch. Just to let existing D8 websites work normally.

Some notes:
- The StringItemBase::isEmpty() only proceeds on the user-added fields having "field_" prefix.
- I was not able to use $this->getFieldDefinition()->getName() because in some cases, the "field_name" key was missing in the definition and the BaseFieldDefinition::getName() produced a notice. (Maybe that's a sign of another bug?)
- Please note that I used $value['value'] instead of $this->value (as it was done in previous patches). Probably this was changed recently?

Let's see if it fails some tests...

It seems that previous patch was green because it did not work at all :D

We shouldn't have a Core class depend on a class provided by field.module.

More generally, it would IMO be really confusing to have a field type behave differently between base and configurable fields.

I'm afraid we need to fix the fails from the previous approach :-\

@yched, as I said in #34, it's just a quick workaround to make life of editors easier right now. Of course it needs to be fixed in some general way.

I agree that such a workaround would be awful.

Regarding #26, I was wondering whether in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord we could store, for
shared tables, the default value of the items? They should still be considered as empty, in case the default value is empty, but then workaround all the various test failures.
I think fixing all those test failures by adding the values in the tests might not be the base idea, given that it could break other existing usecases out there.

Yes, node.title is marked as required but this requiredness is sadly just enforced on the validate method.

Reuploading the latest non workaround patch from swentel #26 so see the test failures.

Duplicate of #2444585: NotNull validation constraint does not fail on empty strings?

+++ b/core/modules/user/user.install
@@ -70,6 +70,7 @@ function user_install() {
+      'name' => 'Anonymous'

Over in #2444585: NotNull validation constraint does not fail on empty strings there is extra code to ensure Anonymous is reserved username - do we need that here - or is unique username enough - guess it depends on case sensitivity

Could we sub-class StringItem as UserNameItem and have a different isEmpty() definition to bypass the crazy in user_install()?

Could we sub-class StringItem as UserNameItem and have a different isEmpty() definition to bypass the crazy in user_install()?

Interesting thought, working on it.

Let's try that out.

I was also working on this and a new user_name field type seems like the easiest way out. The interdiff is to #43 and some more tests are also fixed.

Promoting to critical per #2444585-10: NotNull validation constraint does not fail on empty strings.

1. +++ b/core/modules/user/user.install
   @@ -70,7 +70,7 @@ function user_install() {
   -      'name' => 'Anonymous'
   +      'name' => '',
   

   Is it really needed to have an empty string here?

2. +++ b/core/modules/user/user.module
   @@ -133,6 +133,22 @@ function user_picture_enabled() {
   +  // Allow the 'user_name' field type to use the 'string_textfield' widget.
   +  $info['string_textfield']['field_types'][] = 'user_name';
   ...
   +  // Allow the 'user_name' field type to use the 'string' formatter.
   +  $info['string']['field_types'][] = 'user_name';
   

   Urgs, this is so horrible that we require this

Fixed a few more tests and included the test from #2444585: NotNull validation constraint does not fail on empty strings.

Is it really needed to have an empty string here?

Yes, because we still have 'not null' => TRUE for the database field.

Urgs, this is so horrible that we require this

Uhm.. why? :)

Urgs, this is so horrible that we require this

Agreed, this is one of the reasons why I find "one-off business specific field types" suck (can't look at code atm, so no real opinion on the specific one introduced here, just speaking generally)

A field type whose name is so specific that it looks like the name of a specific field instance is kind of a sign that the specifics would ideally live on that field's definition rather than on a "general field type" used for one field only :-) Dunno if that's possible here though.

But I think a given fIeld definition can specify a different Item class, while using a generic field type, can't it ?

But I think a given fIeld definition can specify a different Item class, while using a generic field type, can't it ?

Yep, that's also an option and it works :)

+++ b/core/modules/user/src/UserNameItem.php
@@ -0,0 +1,29 @@
+    if ($this->getEntity()->id() == 0) {

this would match a string id ('foo' == 0) is TRUE, can we use ===

Once @marthinal has commented can a maintainer add him to the commit credits as bits of our patches from the duplicate #2444585: NotNull validation constraint does not fail on empty strings have been brought over here?

@larowlan many thanks for considering my contribution :)

+++ b/core/modules/user/src/Entity/User.php
@@ -463,13 +463,15 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $user_name_item_definition = $fields['name']->getItemDefinition();
+    $user_name_item_definition->setClass('\Drupal\user\UserNameItem');
+    $fields['name']->setItemDefinition($user_name_item_definition);

wow, I think this is quite a clusterfuck don't you think so? Making it possible to remove the item class like that won't make debuggability easier. It is a string item, but in fact its not really one. I honestly prefer the more explicit variant we had before

Fixes #60 and a lot more tests.

Re #63: No opinion on that, I'm fine with either way :)

Well, I'm not into entity API but IMHO we should avoid changing the used classes for a specific plugin and instead change the plugin ID itself.

I'm done for today...

Adding credit to marthinal, per request from larowlan.

prodding

"Defining a new field type for a one-off use only by a specific field" vs "the definition of that field specifying its different behavior" :
Yeah, I can't say I'm thrilled by either options, ideally user_name string fields wouldn't need that weirdness.

If they really do, then IMO encouraging a pattern of "if one specific field needs some very specific behavior then pollute the list of field types with one-off field types" is a bad pattern to encourage. Field types are supposed to be about reusable, semantic-agnostic data types in a data modelling toolbox.

If some behavior / semantic is specific to a given field, it belongs to that field's definition.
If the behavior can be formulated in a way that makes sense generically for "more that one" field to reuse, then a field type makes sense.

Here, the behavior is "for entity 0, I accept an empty string as a non-empty value", that's ... pretty specific :-)

I *think* @fago added the setClass() methods precisely for this kind of cases, but we could ask for his opinion.

Other (crazy) thoughts :
- This is about user 0 passing validation without having a name, right ? Can't we just say that user 0 name is 'anonymous' internally ? That's what we display anyway, so allowing a non-anon user to chose 'anonymous' as a name sounds like an open door for social trickery anyway ?
- rather than switching a different item class, maybe we could add a $field_definition->setIsEmptyCallback($callable), that would be called instead of the field type's isEmpty() ? Could even happen in a followup ?

More down-to-earth remarks :

1. +++ b/core/modules/user/src/UserNameItem.php
   @@ -0,0 +1,29 @@
   +/**
   + * Defines a custom 'string' field type class for the 'name' user entity field.
   + */
   +class UserNameItem extends StringItem {
   

   If we stick to this approach, the comment should be adjusted, this does not define a "field type".

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -463,13 +463,15 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $user_name_item_definition = $fields['name']->getItemDefinition();
   +    $user_name_item_definition->setClass('\Drupal\user\UserNameItem');
   +    $fields['name']->setItemDefinition($user_name_item_definition);
   

   Minor, but given that getItemDefinition() returns an object which is by ref, we don't really need to re-set it back with setItemDefinition() after modifying it ?
   Could be just $fields['name']->getItemDefinition()->setClass('\Drupal\user\UserNameItem') ?

3. +++ b/core/modules/user/src/UserNameItem.php
   @@ -0,0 +1,29 @@
   +    // Anonymous users do not store anything in the database.
   +    if ($this->getEntity()->id() === 0) {
   +      return $this->value === NULL;
   +    }
   +
   +    return $this->value === NULL || $this->value === '';
   

   The comment a bit brief, so the reason why user_name needs a different isEmpty() logic than the parent remains a bit cryptic. IMO this could use a more detailed explanation. Having not read the recent issue comments fully, I don't really understand it :-)

Also : thanks a TON for picking that one up, folks :-)

Some more fixes.

Need a migrate expert for the migrate fail, and a views expert for the views one - over my head.

The migrate fails are a bug in the setup, specifically for MigrateCckFieldValuesTest so I added the node creations there rather than MigrateNodeTestBase as not to impact the node tests that use the same base class.

I think we need a follow-up though to more gracefully handle nodes with an empty title, which is likely to be common from non-drupal sources.

Arh, just noticed an extra space i removed in MigrateNodeTestBase, can remove that in a future patch.

I think I can fix the Views fails tonight.

Three hours to add two lines to the test...

The Views fail is because EntityViewDisplay::buildMultiple() has a call to FieldItemList::filterEmptyItems(). The user name for uid==0 in the test wasn't being initialized properly, so the new isEmpty() was filtering out this item, so nothing was being rendered. HandlerFieldUserNameTest comes up green for me locally now.

I already have the patch rolled and tested, but it may be better to fix this in UserTestBase::setUp(). Too tired to test try this, but also worried about ripple effects.

Oh, if you are wondering how that gets traced back, start at \Drupal\views\Plugin\views\field::getItems(), go into \Drupal\views\Entity\Render\EntityFieldRenderer::render(), then you end up in ::buildFields, and then you are in the Field API at EntityViewDisplay::buildMultiple(). Whhhhhheeeeeeeeeeee!!!!!!

#72 and #73 still need to be addressed

#72.crazy thought 1: This is my only knowledge reference for why we can't store the anonymous user name in the storage: #2259323-12: Fix the sub-query part in UserSelection::entityQueryAlter.

#72.crazy thought 2: That would make it even harder for debugging, imo.

#73.1, 2 and 3: Fixed.

Created #2550033: Handle empty fields gracefully as the follow-up i mentioned in #76

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php
   @@ -63,4 +63,14 @@ public function preSave() {
   +    $value = $this->get('value')->getValue();
   +    $existing = $this->get('existing')->getValue();
   +    return $value === NULL && $existing === NULL;
   

   IMHO some quick comment here would be great. What makes passwords special

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -463,13 +463,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
          ));
   +    $fields['name']->getItemDefinition()->setClass('\Drupal\user\UserNameItem');
   

   Not a blocker:
   /me still shakes his HEAD, well, this is why Drupal is so complex. Small microopts all over the place

+++ b/core/modules/user/user.install
@@ -70,6 +70,7 @@ function user_install() {
+      'name' => '',

Do we need an update hook for this?

Fixes #84 and #85

Let us fix it.

So we did need the 'name' => ''

Do we need an update path for it - no - that's what my current HEAD install gets for that column,

RTBC+1
Just for the record currently in HEAD the name is empty so no need for upgrade path. Thanks @dawehner & @larowlan for clearing that up.

mysql> select name, uid FROM users_field_data;
+-------+-----+
| name  | uid |
+-------+-----+
|       |   0 |
| admin |   1 |
+-------+-----+
2 rows in set (0.00 sec)

/sorry, misclicked/

Agreed with @dawehner that the "$fields['name']->getItemDefinition()->setClass('\Drupal\user\UserNameItem');" part could use a comment :-)

Other than that, the weirdness of user.name, and the fact that we need to override StringItem::isEmpty(), still bugs me.

- It's currently a non-required field, which implies there was a will to allow it to be empty (as in "no value", not as in "the value is an empty string"), and that default validation will not reject it if empty.
- The fact that we accept it as empty only for user 0 could be enforced by a custom constraint ? (the field already uses some custom business logic constraints)
- user 0 having no name should mean we store NULL, NULL typically means no value, right ? Yet we explicitly enforce "NOT NULL = TRUE" in UserStorageSchema::getSharedTableFieldSchema() (without that special code it would be NOT NULL = FALSE like any field in a shared table)

So it seems we intended to allow NULL user name for user 0, yet we go out of our way to store that as empty string rather than NULL, and then need to add ugly hacks to override the field type's isEmpty() so that in some cases this empty string is not filtered out as an empty value and can reach the storage.

That sounds... backwards :-) Why not just store NULL as you normally would for an empty field, and avoid that growing mess ?

That sounds... backwards :-) Why not just store NULL as you normally would for an empty field, and avoid that growing mess ?

The answer to that is just above the place we declare the 'name' column as not null in UserStorageSchema::getSharedTableFieldSchema():

// Improves the performance of the user__name index defined
// in getEntitySchema().
$schema['fields'][$field_name]['not null'] = TRUE;

Sure, saw that, I'm just saying that it's artificial for a field that is semantically nullable, and forces us to use weird tricks to make it work.

non-null indexes might be more performant, not sure it warrants running through hoops to forcibly make a nullable value not null :-)

[edit : also, the field being non-required and the hoops we use to make it NOT NULL seem to contradict each other]

So it seems we intended to allow NULL user name for user 0 ...

I don't think we ever had that intention, tbh :) As far as I see it, this is a field type where the difference between NULL and an empty string matters (i.e. for db performance reasons).

IMO, the ugly hacks are the other way around: this field should be required, but, if we mark it as such, the NotNull constraint would be added automatically, which by default checks the isEmpty() typed data method, which brings us circling back to the need for a custom field item implementation...

if we mark it as [required], the NotNull constraint would be added automatically, which by default checks the isEmpty() typed data method

Yes, and this patch adjusts isEmpty() to be FALSE if the field contains an empty string, so it means if we go that way, the field always has a value and there's no reason to keep it non-required ? That's why I'm saying we're not being consistent by having the field both non-required and able to have an empty string as an actual value.

this is a field type where the difference between NULL and an empty string matters (i.e. for db performance reasons).

I disagree, that's not a distinction we need here. We're not trying to be able to semantically differentiate between "field has no value" and "field has a value that happens to be an empty string", because we don't have both cases. We have a field that either has a value or doesn't, and in that latter case we'd just prefer to store an empty string instead of NULL. That is just for SQL-specific reasons (NULL and index performances), which should IMO not leak into the data model and field types by introducing IMO potentially highly confusing subtleties (empty string but non-empty Item) if we don't actually have an actual need for that distinction on the data level.

(also, widgets can't really make the difference between "I want no value and I want a value of empty string")

IMO keeping performant indexes could be an optimisation handled at the SQL storage level : for text / varchar properties in fields in shared tables, keep the column as NOT NULL, store empty string instead of NULL, and translate back and forth on load / save. Those considerations shouldn't reach the field type logic.

Can totally be a followup though, so that we can finally get rid of the more general StringItem brokenness, which is a major yay :-)

Ok, let's try and see if anything breaks.

As for the "distinction between NULL and an empty string" part, that also makes sense, thanks for the thorough explanation :)

The other approach could be what we do in #2404331: Can't serialise an entity with an entity reference to an unsaved entity (remove EntityReferenceItem::$NEW_MARKER in favour of a validation constraint) where we use a different data definition, instead of a different data-type. In the alternate data definition we override getConstraints and substitute the NotNull for a different implementation. We could do that here too for name.

Different DataType makes a lot of sense to me. It will also help us remove $fields['name']->getItemDefinition()->setClass('\Drupal\user\UserNameItem'); which is really bugging @dawehner.

Well, the whole point of the discussion since #92 is about finding ways to avoid having to special case a new field type (either an actual field type or a one-off override just for $fields['name']->getItemDefinition()->setClass()).

I'm still kind of convinced about what I wrote in #96 - more briefly :
- We don't need a field type that can be a 3-state ("has a value" / "has an empty value" / "has no value"). That adds unneeded confusion and is not actually what is needed here.
- We just want SQL storage to store "has no value" as empty string rather than NULL for text columns, for SQL-index-specific reasons. That's an optimization that could be done in SQLStorage, probably in a separate issue because we really need to fix the StringItem WTF, which is rightly critical.

So the way I see it it's more a question of "what is less of a WTF til we get there (if we do) ?"
- I'd say it's not creating a new field type, because we couldn't remove it later and would be stuck with it
- The one-off override (as in the current patch) leaks less and would be easier to remove without breaking BC too much

Can't really tell for the approach @larowlan proposes in #98. Where would that "custom definition class" be ? I'm not sure I'm a fan of an approach that would require something lilke :

-    $fields['name'] = BaseFieldDefinition::create('string')
+    $fields['name'] = SomeCustomFieldDefinition::create('string') 

This bugs me less in #2404331: Can't serialise an entity with an entity reference to an unsaved entity (remove EntityReferenceItem::$NEW_MARKER in favour of a validation constraint) where the custom definition class is for a TD property (more "hidden"), but scattering several BFD classes scares me more as a practice to initiate (because it's a more "public" API, or maybe just because it's more "my" API than TD, not sure :-p)

[edit: actually, I know why - there are already several DataDefinition classes, while "there is only one BaseFieldDefinition class" is an epxlicit DX decision that was made a couple years ago now :-)]

1. +++ b/core/modules/content_translation/src/Tests/ContentTranslationMetadataFieldsTest.php
   @@ -61,7 +61,7 @@ public function testSkipUntranslatable() {
   +    $entity_id = $this->createEntity(['title' => $this->randomString(), ], $default_langcode);
   

   I don't think our code style suggest a comma at end of inline arrays.

2. +++ b/core/modules/user/src/Entity/User.php
   +++ b/core/modules/user/src/Entity/User.php
   @@ -463,13 +463,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   
   @@ -463,13 +463,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        $fields['name'] = BaseFieldDefinition::create('string')
          ->setLabel(t('Name'))
          ->setDescription(t('The name of this user.'))
   -      ->setDefaultValue('')
   +      ->setRequired(TRUE)
   

   Would this run into #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state given that it will add NOT NULL to the schema? I guess either we'll need to add the update path here or there depending on which one lands first.

First a reroll because UserUnitTestBase was renamed to UserKernelTestBase.

Fixing #101/1.

I was cross-posting the same patch :)

#101.2: Nope, because that column is already not null; see \Drupal\user\UserStorageSchema::getSharedTableFieldSchema (and the entire discussion above :P)

In that case, looks like we can go back to RTBC. The other concerns seem to me not affecting what is critical about this patch.

Adding credit for yched and jibran for their several helpful comments. And for dmsmidt for reporting.

Overall, this patch looks great.

1. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCckFieldValuesTest.php
   @@ -33,6 +33,35 @@ protected function setUp() {
   +    $node = entity_create('node', array(
   +      'type' => 'story',
   +      'nid' => 2,
   +      'vid' => 12,
   +      'revision_log' => '',
   +      'title' => $this->randomString(),
   +    ));
   +    $node->enforceIsNew();
   +    $node->save();
   +
   +    $planet_nodes = [
   +      4 => 6,
   +      5 => 8,
   +      6 => 9,
   +      7 => 10,
   +      8 => 11,
   +    ];
   +    foreach ($planet_nodes as $nid => $vid) {
   +      $node = entity_create('node', array(
   +        'type' => 'test_planet',
   +        'nid' => $nid,
   +        'vid' => $vid,
   +        'revision_log' => '',
   +        'title' => $this->randomString(),
   +      ));
   +      $node->enforceIsNew();
   +      $node->save();
   +    }
   +
   

   Is this meant to be part of this patch? If so, why?

2. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   @@ -132,7 +132,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setDefaultValue('')
   

   Node::baseFieldDefinitions() still has a setDefaultValue('') on a required field (title). Some other classes also have it on non-required string and email fields. Do some/all of those need this removed as well? Do we need a CR informing contrib to remove it too? What happens if you don't remove it?

Is this meant to be part of this patch? If so, why?

See #76 for an anwser :)

Node::baseFieldDefinitions() still has a setDefaultValue('') on a required field (title). Some other classes also have it on non-required string and email fields. Do some/all of those need this removed as well?

I think @swentel removed it for Shortcut in the initial patches since it simply doesn't make sense to set the default value as an empty string if the field is required. Before this patch, this was useful for nodes because it allowed tests to not specify a value for 'title', but since we have to do that now, we should also remove it from Node.

Do we need a CR informing contrib to remove it too?

It would be a very small one that says to remove it for required string fields. So not sure if it's worth it or not, I'll let others decide :)

What happens if you don't remove it?

Nothing, it just doesn't give you the same benefit as without this patch, i.e. you would still have to explicitly set a value for a required string field.

Looks fine with me again :) I did not track #76 either, makes sense.

This is just a cosmetic change to keep things consistent, so leaving at RTBC.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php
@@ -63,4 +63,16 @@ public function preSave() {
+    $value = $this->get('value')->getValue();

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItemBase.php
@@ -40,4 +40,12 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
+    $value = $this->get('value')->getValue();

+++ b/core/modules/user/src/UserNameItem.php
@@ -0,0 +1,29 @@
+      return $this->value === NULL;
...
+    return $this->value === NULL || $this->value === '';

In UserNameItem we are using magic getter and in StringItemBase and PasswordItem we are explicitly getting values. Can we be consistent here?

Good spot :)

RTBC +1

Just posting some notes for follow-ups if anyone's inspired. Does not need to block the commit of a critical.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php
   @@ -63,4 +63,16 @@ public function preSave() {
   +    // We cannot use the parent implementation from StringItem as it does not
   +    // consider the additional 'existing' property that PasswordItem contains.
   

   I don't think we need this doc. It's pretty clear that the PasswordItem implementation is unique.

2. +++ b/core/modules/aggregator/src/Tests/FeedProcessorPluginTest.php
   @@ -47,9 +47,10 @@ public function testProcess() {
   +    $description = $feed->description->value ?: '';
        $this->updateAndDelete($feed, NULL);
        // Make sure the feed title is changed.
   -    $entities = entity_load_multiple_by_properties('aggregator_feed', array('description' => $feed->description->value));
   +    $entities = entity_load_multiple_by_properties('aggregator_feed', array('description' => $description));
   

   This doesn't seem ideal. I'm not sure if this is a weakness in entity_load_multiple_by_properties() or something else, but it seems like we should be able to loadByProperties from an existing value of the same property without needing to deal with converting one kind of empty to another. Unless there's a quick and obvious answer on why this is good the way it is, if someone can post a followup about that, I'd appreciate it.

3. +++ b/core/modules/book/src/Tests/BookUninstallTest.php
   @@ -57,7 +57,7 @@ public function testBookUninstall() {
   -    $node = Node::create(array('type' => $content_type->id()));
   +    $node = Node::create(array('title' => $this->randomString(), 'type' => $content_type->id()));
   

   A lot of test changes in this patch like this one. Perhaps we need a followup to centralize this somewhere?

4. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCckFieldValuesTest.php
   +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCckFieldValuesTest.php
   @@ -33,6 +33,35 @@ protected function setUp() {
   

   Thanks for the link to #76, but this is not at all easy to understand when just reading the code. Why these specific nids, etc.? Some docs with @see links to where these nids come from and maybe a @todo link to #2550033: Handle empty fields gracefully would be helpful.

5. +++ b/core/modules/user/src/Entity/User.php
   @@ -463,13 +463,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['name']->getItemDefinition()->setClass('\Drupal\user\UserNameItem');
   

   setClass() isn't a method on the @return type of getItemDefinition(), so a /** @var line would help, plus a brief comment explaining why we're doing this would help. Maybe also a follow-up to discuss whether there's a way to not need this?

6. +++ b/core/modules/user/src/UserNameItem.php
   @@ -0,0 +1,29 @@
   +/**
   + * Defines a custom field item class for the 'name' user entity field.
   + */
   +class UserNameItem extends StringItem {
   

   A comment explaining why this isn't in the Plugin namespace would be useful: i.e., that it's not a discovered field type, but an override of the 'string' implementation without changing its type.

Pushed #114 to 8.0.x. Thanks! One or more follow-ups for #116 would be nice, but not critical.

I do think a CR would be good though. Primarily for contrib modules implementing field types that extend StringBase. And secondarily a mention of what a module that does a setDefaultValue('') needs to now know.

I disagree with #116.1. Thanks for the commit @effulgentsia.

Co-wrote this with @gaborhojtsy https://www.drupal.org/node/2554995

I don't know that anyone will actually see this since its been autoclosed but it seems this might be the cause of a pretty serious problem. #2567899: Empty node titles cause failures when migrating because empty strings are treated as NULL I was tempted to mark it as critical. Thanks @dawehner for pointing me to this.