Handle initial values when creating a new field storage definition

Demoting as per #2232477-26: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities ?

When used on a field stored in a dedicated table, supporting an initial value would mean inserting potentially 1000s of new rows.

Once we support 'initial', then ideally, it wouldn't be table mapping dependent. In other words, it should result in the same data state regardless of whether it's in a shared table or a dedicated table (whether that's due to being a bundle field or due to any other table-mapping-specific logic). That would mean that in the case of a dedicated table, when we add the field, we should create records in the dedicated table for every entity that already exists. I discussed this with pwolanin at DrupalCon AMS, and he believes that this is actually a reasonably fast operation (<1 second even for millions of records), if implemented entirely in SQL (not a PHP iteration). I don't know the proper SQL statement to do this, but perhaps pwolanin or someone else can help with that when the time comes.

Once we support 'initial', then ideally, it wouldn't be table mapping dependent. In other words, it should result in the same data state regardless of whether it's in a shared table or a dedicated table

Fully agreed - that's the tricky thing with supporting 'initial' :-)

I discussed this with pwolanin at DrupalCon AMS, and he believes that this is actually a reasonably fast operation (<1 second even for millions of records), if implemented entirely in SQL (not a PHP iteration)

That's good news. But : once we support 'initial', then ideally it wouldn't be storage engine dependant either - meaning, it should be something alternate storage backends (typically, Mongo...) could support as well.

Is "update millions existing documents to add a new sub-entry" something that we can expect to be "reasonably fast" with a Mongo storage ? I guess yes ?

Is "update millions existing documents to add a new sub-entry" something that we can expect to be "reasonably fast" with a Mongo storage ? I guess yes ?

Seems reasonable. I think we can default to the default value when there is no initial value.
I'd love to see this fixed for configurable fields also - e.g. considering REST output after adding a field with a default value, it would make a ton of sense to have that default value in the output for existing entities also.

The related issue affected the similar problem but on field level, comment filed use some hacks to get default value

I am facing this exact issue in #2746279: Adding not null fields to table which have data.

This issue affects the ability to convert a bundle-less entity type into one with bundles. Adding a new bundle field to the existing content entity, then using installFieldStorageDefinition does not work as expected. This is due to the enforcement of all entity key fields need to be NOT NULL (SqlContentEntityStorageSchema::getSharedTableFieldSchema), but the field is initially created as IS NULL and populated with NULL values.

If we had the ability to specify an initial value then the conversion to NOT NULL should go smoother...

Associated: Allow creation of registrant bundles (dpi/rng#108)

There's a way to work around that using a custom storage schema class, see http://cgit.drupalcode.org/file_entity/tree/src/FileEntityStorageSchema.....

#2820848: Make BlockContent entities publishable depends on this.

The ability to use initial values for base fields is badly needed for the Workflow initiative, so I propose to tackle that first in this issue and create a followup for handling configurable fields as well.

Here's an initial patch that will have a lot of fails, so I'm looking for an approval for the direction/approach before spending a lot more time to fix everything :)

Note that it also includes the patch from #2390495: Support marking field storage definitions as required, which is needed for proper handling of 'not null' columns.

I think we identified another thing that needs this in #2820848: Make BlockContent entities publishable, we should initialize the content translation metadata fields with a proper initial value, otherwise we have weird things like a status that is NULL. Not sure if we want to do that here or as a follow-up.

As long as they're base fields (and they are, afaik), I have no problem with handling them in this issue. The final patch will probably be a bit bigger than needed, but I guess that's ok if we get more eyes for reviews :)

Ok :)

The initial problem is partially hidden because content_translation always considers the default translation to be published it seems. But while testing #2820848-71: Make BlockContent entities publishable, I did fine at least one bug when adding new translations of an entity that was created before translations were enabled. That would be an actual bug that we can extend tests for I think.

Here's some fixes for most of the fails.

Patch looks great. So, this means every custom entity has to update the basefield definitions and provide an upgrade path. I think, that is fine.

So, this means every custom entity has to update the basefield definitions and provide an upgrade path.

Not necessarily, we can actually do it for every entity type in a single update function, like in the patch attached.

Also fixed the rest of the fails :)

+++ b/core/modules/node/node.install
+++ b/core/modules/node/node.install
@@ -189,7 +189,9 @@ function node_update_8002() {
   // Regenerate entity type indexes, this should drop "node__default_langcode".
   $manager->updateEntityType($manager->getEntityType('node'));
   // Regenerate "langcode" indexes, this should drop "node_field__langcode".
-  $manager->updateFieldStorageDefinition($manager->getFieldStorageDefinition('langcode', 'node'));
+  $field_definition = $manager->getFieldStorageDefinition('langcode', 'node');
+  $field_definition->setStorageRequired(TRUE);
+  $manager->updateFieldStorageDefinition($field_definition);

Maybe a better fix for this would be to make node_update_8002() depend on node_update_8300().

A few improvements.

+++ b/core/modules/system/system.install
@@ -1794,3 +1796,65 @@ function system_update_8203() {
+          // The 'fid' base field of aggregator items should have been marked as
+          // required in aggregator_update_8001(), but it was not so we need to
+          // correct it here.
+          // @see https://www.drupal.org/node/2602662
+          if ($field_name == 'fid' && $field_definition->getTargetEntityTypeId() == 'aggregator_item') {
+            $field_definition->setRequired(TRUE);
+          }

I've decided to open a separate issue for this: #2840595: The 'Source feed' field of aggregator items has to be updated and marked as required

And for the last 4 fails of the patch above, I opened #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field

Along with #2390495: Support marking field storage definitions as required, we're now blocked on 3 issues here :/

Added PP prefix for easy tracking also the issue summary is quite outdated.

#2390495: Support marking field storage definitions as required is in so we're down to only 2 blockers. However, there's also a new soft-blocker for one of the child issues: #2842480: system_update_8203() is named incorrectly

There's just one dependency left, #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field, so let's how these two patches are working combined.

Discussed this issue with @Berdir and we agreed to split the NOT NULL handling part of this patch to another issue in order to make everything easier to review. Here it is: #2850686: Handle NOT NULL correctly at the storage level for required fields.

After #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field and #2850686: Handle NOT NULL correctly at the storage level for required fields, this simple patch should be all that's needed here!

Quick note to make sure it doesn't get forgotten:

+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -506,6 +506,45 @@ public function setDefaultValueCallback($callback) {
   /**
    * {@inheritdoc}
    */
+  public function getInitialValue() {
...
+  /**
+   * {@inheritdoc}
+   */
+  public function setInitialValue($value) {

Needs an actual doc.

Fixed #38, added support for 'initial values from field' and wrote full test coverage.

This patch is ready for final review IMO, but it still depends on #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

This needed a reroll for the changes in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field and HEAD.

Rerolled for the short array syntax conversion and also made the code that compares the current field schema with the last installed field schema to not take into account any values for 'initial' and 'initial_from_field'.

There is still IS tag on the issue. I think we have a green patch now so we can update it.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1592,20 +1614,48 @@ protected function hasNullFieldPropertyData($table_name, $column_name) {
   +      if (($initial_storage_value = $storage_definition->getInitialValue()) && !empty($initial_storage_value)) {
   

   $initial_storage_value = $storage_definition->getInitialValue() will return false if empty so !empty check is not required.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1592,20 +1614,48 @@ protected function hasNullFieldPropertyData($table_name, $column_name) {
   +        if (!isset($storage_definitions[$initial_value_field_name])) {
   ...
   +        if ($storage_definition->getType() !== $storage_definitions[$initial_value_field_name]->getType()) {
   ...
   +        if (!$table_mapping->allowsSharedTableStorage($storage_definitions[$initial_value_field_name])) {
   

   Can we please add comments to explain the situation in which errors are thrown?

3. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -510,6 +510,85 @@ public function setDefaultValueCallback($callback) {
   +      $properties = $this->getPropertyNames();
   +      $property = reset($properties);
   

   Can we please add a comment here? Can we use $this->getMainPropertyName() here instead?

4. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -510,6 +510,85 @@ public function setDefaultValueCallback($callback) {
   +    if (!is_array($value) || !empty($value)) {
   

   This !is_array check is not needed.

$initial_storage_value = $storage_definition->getInitialValue() will return false if empty so !empty check is not required.

Nope, $storage_definition->getInitialValue() will return an empty array if no initial value is provided.

Can we please add comments to explain the situation in which errors are thrown?

I think each exception message is very clear about the error that triggered it.

Can we please add a comment here? Can we use $this->getMainPropertyName() here instead?

Yes we can, fixed.

This !is_array check is not needed.

It is needed because we want to support passing a string to setInitialValue().

Rerolled the combined patch for the reroll of its dependencies.

Oh, and we have a new dependency: #2615496: A serial/primary key field can not be added to an existing table for some databases :/

Marked #2274597: Not all required fields are marked as required as a duplicate of this issue as per discussion with other committers and the entity and field maintainers back in December. We also agreed that this issue is major priority and probably can be considered a bug.

I also have some note about adding the "related block content issue etc." but I have no idea what that means, so hopefully someone else does. :)

#2615496: A serial/primary key field can not be added to an existing table for some databases is in. We're getting closer :)

Let's update the issue summary in meanwhile.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1592,20 +1614,49 @@ protected function hasNullFieldPropertyData($table_name, $column_name) {
+        $storage_definitions = $this->entityManager->getFieldStorageDefinitions($storage_definition->getTargetEntityTypeId());
+        if (!isset($storage_definitions[$initial_value_field_name])) {
...
+        if ($storage_definition->getType() !== $storage_definitions[$initial_value_field_name]->getType()) {
...
+        if (!$table_mapping->allowsSharedTableStorage($storage_definitions[$initial_value_field_name])) {

This should use $this->fieldStorageDefinitions per #2867325: SqlContentEntityStorageSchema needlessly fetches storage definitions it already has.

I hit this due to a problem with this patch. See the attached patch for the (minimally-invasive) workaround I employed that fixed this for me. Would love to hear your thoughts on this.

I think an explanation of our concrete example is the best way of understanding the problem:

We currently have an entity type that has a status field that is a string field. Following Drupal 8.3's best practice we are now adding a boolean status field from EntityPublishedTrait to all of entity types. So I want to rename our existing string field (to processing_status in this particular case) and then add the standard boolean status field. My update hook to achieve this conceptually looks like the following:

$storage_definition = BaseFieldDefinition::create('string')
  ...
  ->setInitialValueFromField('status');
$entity_definition_update_manager->installFieldStorageDefinition('processing_status', $entity_type_id, $provider, $storage_definition);

// Run DB queries to empty the data from the "leftover" status column(s).
db_update(...);

$old_status_field = $entity_definition_update_manager->getFieldStorageDefinition('status', $entity_type_id);
$entity_definition_update_manager->uninstallFieldStorageDefinition($old_status_field);

$new_status_field = BaseFieldDefinition::create('boolean')
  ...;
$entity_definition_update_manager->installFieldStorageDefinition('status', $entity_type_id, $provider, $new_status_field);

The problem is that when SqlContentEntityStorageSchema validates the "from" field and makes sure that the field types of the new field and the "from" field match that does not fetch the last installed field definition of the "from" field, but the one generated from the new code. And in my case the status field is already a boolean in code even though at this point in the update function it should still be considered a string, and the last installed field storage definition rightly is still a string at this point.

So I solved this by allowing the field storage definitions to be set in the storage schema and then setting them from FieldStorageDefinitionListener::onFieldStorageDefinitionCreate() (see the attached patch). Not sure if there are more elegant ways of solving this. Note also (as I also noted in the code comment in the patch) this would become a non-issue with #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code.

To come around full-circle: my patch only works if SqlContentEntityStorageSchema actually uses the field storage definitions that it has as a class property (and that can be set externally with my patch).

Oops, here's the patch I promised.

This also needed a re-roll for #2856808: Break out the 'entity_test_update' entity type into its own module and add additional test db dumps.

And here, finally, is an interdiff for the additional change I mentioned at the top of ##4. I'll shut up now...

OK, one more... ;-) Here's a combined patch for the one #51 to see if it's still green.

OK, now back to needs work for #49.

#2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field was discussed in depth during a recent Workflow Initiative meeting at Drupalcon Baltimore and everyone agreed that it is a risky change which will require per-entity type updates and a *lot* of work for keeping BC. Since that will take quite some time to implement properly, I had to take another look at the code in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema() and realized that the dependency on that issue was there simply because I started working on it from that angle, by fixing NOT NULL handling first.

But this API addition can be done without it, even as the code comment says:

// @todo Revisit once we have support for 'initial' in
//   https://www.drupal.org/node/2346019.

So let's get initial values support in first and fix NOT NULL handling as the followup.

I agree with #49 so here's a reroll of #44, which also includes the interdiff from #52.

Do you have any thoughts about #50?

@tstoeckler, the funny thing is that I hit a very similar bug in #2860654-9: Field storage CRUD operations must use the last installed entity type and field storage definitions, which is best explained by the code comment I wrote for that issue:

    // Entity type definition updates can change the schema by adding or
    // removing entity tables (for example when switching an entity type from
    // non-revisionable to revisionable), so CRUD operations on a field storage
    // definition need to use the last installed entity type schema.

If we combine that with the problem you explained in #50, the only conclusion is that any CRUD operation of field storage definitions need to use the last installed entity type definition *and* the last installed field storage definitions.

Tempted to RTBC but I think this needs another review / feedback on #57 from @tstoeckler.

Also, I guess we need a change record?

Here's a change record for this: https://www.drupal.org/node/2877964

As for #49/#50, I tried various approaches in #2860654-11: Field storage CRUD operations must use the last installed entity type and field storage definitions but, as I said there, we need to fix the entity update system as a whole so the only thing we can do in this patch is use the last installed definitions directly when validating the "from" field.

Also updated the issue summary so this should be good to go :)

@amateescu, looking at the Change Record I am not sure if the initial value has to be set only in the update function installing a new base field or in the base field definition in the entity class as well. Because the definition is cached I would say it has to be provided at both places otherwise there will be a mismatch between the installed definition and the one from code.

@hchonov, that's a good point. The patch already takes care of that and strips the "initial value" stuff from the definition before caching it, so you only need to specify it in the update function.

Updated the CR to reflect this. Thanks for the pointer! https://www.drupal.org/node/2877964/revisions/view/10481455/10481577

Can't see any reason not to RTBC.

Sorry, but I think #50 is still a needs work. As you do in #2860654-11: Field storage CRUD operations must use the last installed entity type and field storage definitions I think for this to go in we need to make the StorageSchema always use the last installed field definitions and I don't think that should be postponed on any issue as it will cause trouble for people doing what I tried to do in #49, especially as there really is no workaround.

Oh wait, sorry, #59 already does that. I will verify that that fixes my issue that I had above.

Yay, my test worked! Back to RTBC now. Thank you all for your patience and for you great work on this!

I attached some files for folks playing along at home. To reproduce what I did, download all the files from this comment + patch #59 from this issue into your Drupal root and then run the following from there:

# First apply the patch
git apply 2346019-59.patch
# Apply the patch that makes the status field on nodes a string field
git apply 2346019-before-update.txt
# Setup the database with a fresh install and some example content
drush site-install
drush php-script 2346019-generate-content.txt
# At this point there should be a string 'status' column with some values in node_field_(data|revision)
# Now run the update
git apply 2346019-update.txt
drush updatedb
# Now the 'status' column is a boolean field with all values being 1 and there
# is a new 'status_string' column with the values previously in the 'status' column

Oops, forgot the two other files.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1102,6 +1107,23 @@ protected function processIdentifierSchema(&$schema, $key) {
   +        unset($field_storage_schema[$table_name]['fields'][$key]['initial']);
   +        unset($field_storage_schema[$table_name]['fields'][$key]['initial_from_field']);
   

   fyi unset takes multiple args.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1613,29 +1635,58 @@ protected function hasNullFieldPropertyData($table_name, $column_name) {
   +        $initial_value = $initial_storage_value[0];
   

   Can we add a comment here as to why we get the first value of the array. Personally I prefer reset() over explicitly using 0 but thats just a preference. What happens if an initial value needs to be set as multiple values? Not supported? Do we validate that?

3. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -510,6 +510,83 @@ public function setDefaultValueCallback($callback) {
   +   *   - a numerically indexed array of items, each item being a property/value
   +   *     array;
   

   This suggests that we support multiple values? Yet earlier we explicitly use delta 0? Am I missing something? Note: not mentioned in change record

4. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -510,6 +510,83 @@ public function setDefaultValueCallback($callback) {
   +      $value = [];
   +    }
   +    // Unless the value is an empty array, we may need to transform it.
   +    if (!is_array($value) || !empty($value)) {
   

   Why not set the initial value and return when we know its an empty array. Would make the logic simpler. Early returns++

5. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -510,6 +510,83 @@ public function setDefaultValueCallback($callback) {
   +      elseif (is_array($value) && !is_numeric(array_keys($value)[0])) {
   

   We only check the first key here? What if someone passes mixed keys? Wouldn't it be better to use array_filter with is_numeric

6. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -510,6 +510,83 @@ public function setDefaultValueCallback($callback) {
   +    return isset($this->definition['initial_value_from_field']) ? $this->definition['initial_value_from_field'] : NULL;
   

   Oh for php7 and null coalesce :)

7. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -817,4 +818,119 @@ public function testLongNameFieldIndexes() {
   +      ->setInitialValue('test value');
   

   Shouldn't we be adding a test for the other formats too?

8. +++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
   @@ -195,6 +195,52 @@ public function testFieldDefaultValue() {
   +    // Set default value with an indexed array.
   +    $definition->setInitialValue($expected_default_value);
   +    $this->assertEquals($expected_default_value, $definition->getInitialValue($entity));
   

   Do we need a test for multi-value field?

@larowlan, thanks for reviewing!

Re #67:

1. I know it does but in this case I think the code is more readable when separated on two lines.

2. Added a comment and opened a follow-up issue for supporting configurable and multi-value fields: #2883851: Add initial values support for configurable and multi-valued fields

3. I would like to see if we can support multiple values or not in the followup mentioned above, so I'd prefer not to lock down the API (even through documentation) until we know for sure.

4. 5. That's a direct copy of \Drupal\Core\Field\BaseFieldDefinition::setDefaultValue(), so let's open a followup to improve both if you feel strongly about it :)

6. Yeah :P

7. We are testing other formats in \Drupal\Tests\Core\Entity\BaseFieldDefinitionTest::testFieldInitialValue()

8. Nope, but #2883851: Add initial values support for configurable and multi-valued fields should add one.

A local testing change slipped in #68. Putting back to RTBC because the only difference to #59 is the code comment added in the interdiff from #68.

Those were some CI errors.

I thought I'd posted a review on here already but apparently not, just to say it looks good to me, giving larowlan a chance to get back on it.

Don't hold back on my behalf

Discussed this briefly with larowlan, he mentioned possibly throwing an exception from setInitialValue() until #2883851: Add initial values support for configurable and multi-valued fields is done, that seems like it'd make what's supported clearer until the follow-up gets resolved.

I'm 50/50 on the early return - I really like them on longer methods, but less so when the method is already short.

Sure thing, I don't mind throwing an exception there for now :)

Thanks, +1 RTBC

Committed 29b7ea9 and pushed to 8.4.x. Thanks!

Thanks everyone for the reviews! The next one in line is #2854732: Use initial values for content translation metadata fields and fix existing data and we're just two issues away from being able to convert core's entity types to revisionable and publishable :)

This also unblocked #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

So this causes an interesting problem, specifically, it results in an entity schema definition mismatch on all fields that had previously manually set an 'initial' value through a custom StorageSchema handler.

Because what happens now is that we remove those two keys from the *generated* schema but it might still exist in the stored installed schema.

I've seen this so far in paragraphs (status, behavior_settings) as well as in file_entity (type). I think the solution is to process both the stored and the generated schema equally, to consistently ignore that value, or explicitly process the installed schema and remove it from that? I'll open an issue for that.

Created #2925550: Fields with a manually defined initial value in the schema have an Entity schema definition mismatch after updating to Drupal 8.4