Changing cardinality lower than highest existing delta causes data loss upon save

This is not major, the lack of a message. There are many other cases in Drupal where we dont provide a warning, although it changes existing data.

If the person changes it back to three do the values re-appear?

I'm wondering what will happen in my case. I have a image field, set to unlimited entries.

People upload photos to this field and then using the insert module they add them to their body.

Now if I reduce the allowed values to 1 and they edit->save their post the other image field entries will be lost. Does this also mean the original images uploaded in those entries will be removed? Or will it be just the entries themselves? Because if the images are removed then this will also "break" the body content they have created.

Sorry if this is too tangential, I'd be happy to open a new issue if you think it is.

This is really major.

The easiest way is to remove the save button when we know there's data. Question is if our API's also should enforce this or not.

We already display a big warning (even though it doesn't prevent actually changing the number of stored values).

Is that not sufficient here? Seems at least this could be 'normal'.

A possible simple solution that would allow downgrading this issue to normal:
change the warning message to say "There is data for this field in the database. Changing field settings may cause loss of data."

I think this still deserves to be major though, data loss is bad. Will have a look at it during the sprint.

Test fail patch

And the fix.

The function getHighestDelta() was lying, so changed that, Interdiff is against #17

Fix the placeholdr

Here's a small review :)

1. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageInterface.php
   @@ -29,4 +29,14 @@
   +   * Get the highest delta for a field.
   

   Gets.. :)

2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageInterface.php
   @@ -29,4 +29,14 @@
   +  public function getHighestDelta($storage_definition);
    }
   

   Missing empty line.

3. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -691,6 +691,16 @@ public function hasData() {
   +    return \Drupal::entityManager()->getStorage($this->entity_type)->getHighestDelta($this, TRUE);
   

   What's up with the extra 'TRUE' param?

4. +++ b/core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
   @@ -153,6 +153,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      $highest_delta++;
   +      if ($highest_delta > $form_state->getValue('cardinality_number')) {
   

   We could inline this to:

   if (++$highest_delta > $form_state->getValue('cardinality_number')) {
   

5. +++ b/core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
   @@ -153,6 +153,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +        $form_state->setErrorByName('cardinality_number', $this->t('You can not set the cardinality lower than @delta', array('@delta' => $highest_delta)));
   

   How about tweaking the error message a bit to something like:

   "This field already contains data so a cardinality lower than @delta can not be set."

   I mean.. it would be nice to tell the user why they can't change this to a lower value.

1. fixed
2. fixed
3. doh - copy paste, fixed
4. done
5. good suggestion

- edit - cleaned up the tests too.

Wondering whether Mongo storage will be able to support getHighestDelta() :-)

And also, even with the SQL storage, if the query will scale large datasets ?

Wondering whether Mongo storage will be able to support getHighestDelta() :-)

Well, I think mongo has the same problem with countFieldData() :)

And also, even with the SQL storage, if the query will scale large datasets ?

Hmm, good point. From Mysql.

MySQL uses indexes for these operations:

To find the MIN() or MAX() value for a specific indexed column key_col. This is optimized by a preprocessor that checks whether you are using WHERE key_part_N = constant on all key parts that occur before key_col in the index. In this case, MySQL does a single key lookup for each MIN() or MAX() expression and replaces it with a constant.

mysql> EXPLAIN SELECT max(delta) FROM node__body;
+----+-------------+------------+-------+---------------+-------------+---------+------+------+-------------+
| id | select_type | table      | type  | possible_keys | key         | key_len | ref  | rows | Extra       |
+----+-------------+------------+-------+---------------+-------------+---------+------+------+-------------+
|  1 | SIMPLE      | node__body | index | NULL          | revision_id | 4       | NULL |    1 | Using index |
+----+-------------+------------+-------+---------------+-------------+---------+------+------+-------------+

There's no direct index on delta, but it's using the revision_id since that is part of the primary key, so I guess we're probably fine?
Also, changing the cardinality when you already have a lot of data won't be a day to day operation I presume.

Not sure about the SQL reasoning in #28 - according to the SQL explanation (which is what could be expected), an index can be used for queries like
SELECT MIN(index_part2), MAX(index_part2) FROM tbl WHERE index_part1=10;
(i.e all index parts before the column used for MAX are fixed by constants)

Thus, for a query like
SELECT max(delta) FROM node__body
without fixing any other column (we're looking at all revisions on all entities), the only possible index would have to be an index directly on delta ? Without this, I can't see how the query could avoid scanning all rows ?

(side note : shared tables can only hold one delta by definition, so I don't think a query is needed in that case ?)

More genrally - yeah, tough issue...

Other thoughts :
- forbidding the delta lowering altogether could be very frustrating. Some entity somewhere (guess which one...) has more deltas, so you're screwed ? (and if that's in an old revision you cannot even edit, same thing ?). Maybe it's not really worse than than the stuff we currently forbid if hasData(), not sure...
- with CMI deployment in D8, such checks on the current content of the db are awkward anyway, since, if that check is performed on form validation only and not on actual config change, that still leaves the case where you make the change on your dev site with a possibly smaller database, and then deploy it on prod (which is going to be pretty common IMO). That argument also applies to the stuff we currently allow based on hasData() (changing some storage settings), but our case here is even more fragile (more chances that the change would be allowed on the dev db but forbidden on the prod db). In other words, there's always a gap we can't really fill :-/
- one confusing thing with our current keeping the old overflowing deltas untouched in the db until next time the entity is saved, is that the values still match with Views queries or EntityQueries (and when you actually load the entity its content doesn't match anymore)

Alternatively, what if :
- We allow the config change, and mark all corresponding deltas as deleted = 1 for proper "deletion" in the purge process (that still requires a "WHERE delta > N" query that would require a dedicated index...). That would mean changing the algorithm for purge (currently only looks at fields or storages that have been deleted completely).
- Maybe warn the user / add a confirmation step if he sets the delta to a lower value than the current max. Not sure about that, since that's still prone to the "apply on dev db / deploy on prod" drawback. So maybe always warn if he lowered the delta (existing data or not) ? Or just some alarming #description on the delta dropdown in the form ?

Also: this definitely is an issue (a complex one), but I'm not sure we have to rush a fix pre-RC ? :-) This data drop has been there since the beginnings of CCK, and I don't think I have seen lots of complains ? Not saying this wouldn't need fixing, of course...

No rush indeed, and very good points, will let them sink a bit :)

This patch slightly improves #22 by adding form validation before submit, and a #description informing the user beforehand that he can't set it to less than highest delta.

But indeed the problem remains that there's no easy solutions to this. I think that the config apply is probably not solvable at all, and it's probably a larger problem where this is just a sub-set. If you truncate the field data table, you're allowed to edit all field settings (even the ones disabled by hasData())and then exported to live and ruin all data. My proposal would be to move that scenario (TRUNCATE table + field config edit + config export + config import) to a separate issue.

On this issue, we should deal on preventing data loss via the UI, and for this I see some solutions:

1. Allow setting number of allowed values up to highest delta (current patch). This has the problem that it can lead to data loss due to highest delta being lower on development environments than in live (I expect that to happen frequently).
2. Allow changing the number of allowed values only to higher than the previous value. This solves the problem above, without having to add a getHighestDelta() function, as it should never be possible to get below highest delta this way. This would make it impossible to change away from 'Unlimited'
3. Same as for the rest of the field settings, if hasData() is TRUE, simply disable the ability to change the number of allowed values.

Personally, I would vote for 2, due to simplicity, and the ability to add a few more values to a field, if deemed necessary.

Too much time with D7 lately. This should fix the validation test error, as this is now validated using the HTML minimum. Also changed to short array syntax.

Definitively need to run these tests locally before updating the patch.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1656,6 +1656,49 @@ public function countFieldData($storage_definition, $as_bool = FALSE) {
   +    elseif ($table_mapping->allowsSharedTableStorage($storage_definition)) {
   +      // Ascertain the table this field is mapped too.
   +      $field_name = $storage_definition->getName();
   +      try {
   +        $table_name = $table_mapping->getFieldTableName($field_name);
   +      }
   +      catch (SqlContentEntityStorageException $e) {
   +        // This may happen when changing field storage schema, since we are not
   +        // able to use a table mapping matching the passed storage definition.
   +        // @todo Revisit this once we are able to instantiate the table mapping
   +        //   properly. See https://www.drupal.org/node/2274017.
   +        $table_name = $this->dataTable ?: $this->baseTable;
   +      }
   +      $query = $this->database->select($table_name, 't');
   +      $query->addExpression('MAX(delta)');
   +    }
   

   shared storage fields do not have delta. That means you can just return 0 here?

   At the same time, it is not possible to change cardinality for such a field, so calling it for that doesn't really make sense in the first place.

2. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -705,6 +705,16 @@ public function hasData() {
   +   * Determines the highest delta for a field.
   +   *
   +   * @return int
   +   *   The highest delta.
   +   */
   +  public function getHighestDelta() {
   +    return \Drupal::entityManager()->getStorage($this->entity_type)->getHighestDelta($this, TRUE);
   +  }
   

   This would need to be added to the interface for FieldStorageConfig too.. or we just leave it out and the single place that needs it calls it directly?

We ran into this issue during an internal sprint @Happyculture.
What we think is, like @yched said, that the functionnality of being able to reduce the cardinality is more interesting than the risk of data loss. If the field data were dropped at the moment the field storage settings were changed, it wouldn't be the same, but currently that only occurs when an existing entity having values than the cardinality is saved.

What we might do is the ask for confirmation only when some fields have more values than the new cardinality.

Our 2 cts :)

Cleaning up drupalcampfi tags.

The committers discussed this with the entity and field API maintainers, and we agreed to upgrade this issue to critical because it causes data loss (even though it is rare).

The proposed resolution would continue to limit the flexibility for site builders. As a site builder I prefer to have as much flexibility as possible so that I am not locked in once I click Save on a field.

The good thing is that at least the patch in #34 would allow a contrib module to reverse this behavior and provide a warning instead.

As a site builder I prefer to have as much flexibility as possible so that I am not locked in once I click Save on a field.

You are not locked in when you create a field, only when you add some content in that field.

You're right. But I still prefer it when Drupal does not lock a site builder out after content is created, and at least the behavior can be reverted in contrib at the very least.

@mradcliffe it would only prevent you from reducing the cardinality to the point where you'd lose data. We could potentially show the offending entities too.

Now that #2384459: Add entity query condition for delta in EFQ is in 8.2.x this is unblocked.

I think we should go ahead and fix this issue using the entity query delta support here in 8.2.x

We can then evaluate both issues for 8.1.x - I moved the other one down to 8.1.x and postponed it on this.

This is a new patch that uses the new entity query functionality.

It provides less value to the user, we can't set it upfront as the client side form validation. To make up for it, we can give the user an idea of how many entities there are.

Also extend the tests a bit to check for the format plural strings and make sure we can set it to the highest delta but not below.

No interdiff since it has almost nothing in common with the previous patch.

We must not run that query for new fields.

I'm neither for nor against the patch, just wanted to remind a comment made earlier, that this approach could allow a cardinality change on a dev site with fewer content, and then let it be deployed on prod, where it might have been rejected if done directly on the prod backend UI because the prod site has more content.

If the data loss that currently happens when cardinality gets lowered is considered critical, the patch here is more a partial safeguard than a fix. There can still be data loss, depending on the site instance on which the config change is attempted.

@yched: So you're neutral? How swiss of you ;) Yeah. I guess we could look into config sync validators if we think this is important enough. But I'd only consider that if those are capable of only doing the check if we can see that it changed, otherwise it would be a lot of queries all the time.

#2384459: Add entity query condition for delta in EFQ is not yet in 8.1.x, so obviously the tests can't work there.. added one more check to avoid the query for new entities, running the test with 8.2.x for now.

Opened #2726415: Add configuration sync validation. Like the approach here, but moving it out of form validation would be good for cases like the one yched mentions.

Looks good to me, only one nitpick, can be fixed on commit.

+++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
@@ -267,6 +267,23 @@ function cardinalitySettings() {
+    // Assert that you can't set the cardinality to a lower number then the

then = than

ONLY fixed swentel's nitpick.
No other changes to the diff file.

Fixed this phpcs nit on commit to 8.2.x

FILE: ....x/core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 164 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Moving back to 8.1.x.

Unpostponed #2384459: Add entity query condition for delta in EFQ. But now this issue gets to be postponed on that!

ManagedFieldsTest is broken on PostgreSQL since commit.
Please see PG testrunner

[18-May-2016 19:09:15 Australia/Sydney] Uncaught PHP Exception Drupal\Core\Database\DatabaseExceptionWrapper: "SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "number"

+++ b/core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
@@ -152,6 +152,22 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
     if ($form_state->getValue('cardinality') === 'number' && !$form_state->getValue('cardinality_number')) {
       $form_state->setErrorByName('cardinality_number', $this->t('Number of values is required.'));
     }
+
+    // Validate that there are no entities with a higher delta.
+    $field_storage_definitions = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions($this->entity->getTargetEntityTypeId());
+    if (!$this->entity->isNew() && isset($field_storage_definitions[$this->entity->getName()]) && $form_state->getValue('cardinality') != FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED) {
+
+      // Get a count of entities that have a value in a delta higher than the
+      // one selected. Deltas start with 0, so the selected value does not
+      // need to be incremented.
+      $entities_with_higher_delta = \Drupal::entityQuery($this->entity->getTargetEntityTypeId())

Looks like this can happen when cardinality is set to number, then the actual number is in a separate field.

Should be a pretty easy fix, we just need to check that above, put the actual cardinality in $cardinality and use that.

Also means we should extend the test with one case that uses that as that wouldn't work right now.

Moving back to 8.2.x for that, we're blocked on 8.1.x anyway.

Reverted in 8.2.x

@alexpott, @catch, @effulgentsia, @Cottser, @webchick, and I agreed this issue remains critical because of the data loss.

(Removing outdated beta eval.)

Actually, we already test that and cardinality *is* the number.. except when no number is provided. So just making sure that we don't run the query then.

Looks good to me.

Committed 2486f10 and pushed to 8.2.x. Thanks!

Back to RTBC against 8.1.x again.

I think testbot got confused with the minor version, re-uploading a the patch for 8.1.x.

I can't reproduce the failures locally, so sent for re-test.

Well, #2384459: Add entity query condition for delta in EFQ is not in yet :)

No longer blocked.

Now the other patch was actually pushed, patches are green now.

This is a reroll of a patch already in 8.2.x - looks great - thanks @Berdir

Committed 3280164 and pushed to 8.1.x. Thanks!

#2760279: Reloading edit form can cause accidental data deletion

Posted this followup: #2895124: Field storage settings give a scary red warning that they can't be changed when the cardinality is the only thing with restrictions