File usage for inline images is only recorded for first field item

Amazing find!

This is the culprit:

$text = $entity->get($formatted_text_field)->value;

This does not specify a delta, and so it only access the first item (delta zero).

Note that we do have test coverage for this, we just didn't think about multi-valued formatted text fields.

It's indeed a nice catch casey! I think I got it working but still needs the tests.

PS: This is my first patch ever to Drupal so please be kind :)

Great @thpoul!

+++ b/core/modules/editor/editor.module
@@ -463,8 +463,12 @@ function _editor_get_file_uuids_by_field(EntityInterface $entity) {
+    } ¶
+    $uuids[$formatted_text_field] = _editor_parse_file_uuids($text);   ¶

Has Whitespace warning upon applying #3 patch. Uploading a new patch by removing the whitespaces.

Tested with the below SQL,

SELECT fm.*, fu.* FROM `file_managed` AS fm LEFT JOIN `file_usage` AS fu ON fm.fid = fu.fid;

This SQL should not return any NULL value from file_usage table.

@WimLeers, I think you are referring to \Drupal\editor\Tests\EditorFileUsageTest? I'll have a look at the test suite.
Thanks!

@aneek oh my editor did it again!

@WimLeers do you think it should be tagged "rc target" ?

Fix looks fine. Still needs tests though.

@aneek: yep, that's the one!

@thpoul: yes, I think this will qualify. To some extent, this is data loss. Tagging accordingly.

+++ b/core/modules/editor/editor.module
@@ -463,7 +463,11 @@ function _editor_get_file_uuids_by_field(EntityInterface $entity) {
+    $formatted_text_field_children = $entity->get($formatted_text_field);

Let's rename this variable to $field_items. "children" is the wrong term here.

Updated IS to justify rc target triage.

Changed variable name as requested to $field_items

Looks good. Still needs tests.

Updating status for #13.

Tests added :) Hopefully everything is ok. Crossing fingers!

Something odd is going on here. If I apply just the new tests (the interdiff) against an unpatched 8.0.x then the tests pass, which they shouldn't...

@thpoul, great! But can we have a "test-only" patch? This will help us to understand the test and the fix in a better way.
Thanks!

@blackra just realized that the test is only checking multiple inline images in the body field without unlimited cardinality (as described from the op)

I'm writing a new test now

Great, thanks! Will review the next patch when it's ready :)

I have an issue which I don't seem able to figure out. The test fails now for the images of the additional body items.

PS: for some reason the files are cached try uncaching the url to view the actual files correctly

The patch isn't relative to latest HEAD; it includes a revert for the last commit to Drupal 8 (#2609110: Update Twig to 1.23.1).

Try these:
https://www.drupal.org/files/issues/2574737-20-do-not-test.patch?1
https://www.drupal.org/files/issues/interdiff-2574737-15-20.txt?1

I think @thpoul has uncovered a deeper Entity API bug. Potentially a critical bug.

It seems that when update hooks are called, only the first field item is present, even though there are three field items!

Attached patch adds some debug output that makes this very easy to debug. Raising to critical to bring it to the immediate attention of Entity/Field API experts. If my suspicion is correct, this will of course need to be split off to a separate issue, but given we're so close to RC4, I think it's worth making this issue critical because it makes it very easy to reproduce the problem: just run EditorFileUsageTest (a very fast test) and observe the problem: all debug output says 3, except in one place, where it says 1.

Review of the patch for @thpoul:

1. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -56,33 +56,57 @@ protected function setUp() {
   +      // Store the image entity in an array for later use
   

   Comment can be removed; this is clear from context.

2. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -56,33 +56,57 @@ protected function setUp() {
   +      // Don't be rude, say hello
   

   Missing trailing period.

3. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -56,33 +56,57 @@ protected function setUp() {
   +      // Test handling of a valid image entry
   

   Missing trailing period.

4. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -56,33 +56,57 @@ protected function setUp() {
   +    ¶
   

   Trailing whitespace.

5. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -90,45 +114,72 @@ public function testEditorEntityHooks() {
   +    for ($i=0; $i < sizeof($image_entities); $i++) { ¶
   

   Trailing space.

   Also, we don't ever use sizeof(). Use count() instead.

   Finally, $i=0 -> $i = 0.

6. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -90,45 +114,72 @@ public function testEditorEntityHooks() {
   +      $new_value = str_replace('data-entity-type', 'data-entity-type-YOLO', $original_value);
   

   s/YOLO/modified/ — no need to change this from what it is in HEAD.

IMHO having test coverage that we don't loose files for itself is pretty significant.

Cannot check myself atm. Wondering, though : the test and steps to reproduce in the IS only seem to mention the case of images embedded into a text editor on a formatted text field ? Is that specific to this case ? (as opposed to images directly in an image field ?)

False alarm, everything is fine with Entity API, just a bit confusing.

Add this to setUp():

FieldStorageConfig::loadByName('node', 'body')
  ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED)
  ->save();

and the test passes.

Also, I would recommend that you rely on the Entity API magic methods to simplify your code, makes it alot easier to read, I initially thought something is wrong with your code there in the loop that changes the value:

$body = $node->get('body')->get($i)->get('value');
$original_value = $body->getValue();
$new_value = str_replace('data-entity-type', 'data-entity-type-YOLO', $original_value);
$body->setValue($new_value);
      $original_values[$i] = $original_value;

It's quite confusing what $body actually is in this scenario (it's not the body field, it's just the string of it). This is IMHO much easier to read, partially because I'm used to it:

$original_value = $node->body[$i]->value;
$new_value = str_replace('data-entity-type', 'data-entity-type-YOLO', $original_value);
$node->body[$i]->value = $new_value;
$original_values[$i] = $original_value;

See also http://drupal.stackexchange.com/a/180393/31 for a recent DA answer that I wrote on that topic.

Darn. That crossed my mind, but I dismissed it based on the fact that everywhere else the expected number of field items exists. I guess Entity/Field API simply do not *enforce* the configured cardinality (or even validate it). I think this is the perfect example of where assertions would be valuable?

In any case, many thanks for checking this, Berdir, and sorry for the false alarm!

Hah, nice job @Berdir :-)

Yeah, entity/field API doesn't prevent extraneous deltas in runtime objects. They just don't get stored (but IIRC that's in the hands of the storage layer) I *think* validation would fail too, but validation is not enforced.

I don't think we ever really discussed enforcing the max delta in runtime FieldItemList objects, nor do I have a clear vision of what the impact would be.

Hence why I think an assert() makes sense. No impact in production. Developers warned in development.

After @WimLeers and @Berdir great input I am attaching what aims to be the last patch for this issue :D

Lots of silly whitespace problems. Easy fixes.

1. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -39,6 +41,11 @@ protected function setUp() {
   +    FieldStorageConfig::loadByName('node', 'body')
   +        ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED)
   +        ->save();
   

   These are indented by 4 instead of 2.

2. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -49,6 +56,7 @@ protected function setUp() {
        $type->save();
   +
        node_add_body_field($type);
   
   @@ -56,33 +64,57 @@ protected function setUp() {
        ));
   +
        $node->save();
   

   Unnecessary newline addition.

3. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -56,33 +64,57 @@ protected function setUp() {
   +    }
   +    ¶
        // Test editor_entity_insert(): increment.
   
   @@ -90,45 +122,71 @@ public function testEditorEntityHooks() {
   +    }
   +    ¶
        $node->save();
   

   Trailing whitespace.

4. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -90,45 +122,71 @@ public function testEditorEntityHooks() {
   +    // // Test hook_entity_update(): increment, by modifying the last revision:
   +    // // read the data- attributes to the body field.
   

   Double comment indentation :P

Other than that, this looks great.

Hopefully the curse my ide has, has been removed.

Looks great! Thank you very much, a very useful first Drupal core contribution! :)

Discussed with xjm and tagging as RC target due to data-loss potential.

+++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
@@ -56,33 +63,56 @@ protected function setUp() {
+      $this->assertIdentical(array(), $file_usage->listUsage($image), 'The image has zero usages.');
...
+      $this->assertIdentical(array('editor' => array('node' => array(1 => '1'))), $file_usage->listUsage($image_entity), 'The image has 1 usage.');

@@ -90,45 +120,68 @@ public function testEditorEntityHooks() {
+      $this->assertIdentical(array('editor' => array('node' => array(1 => '3'))), $file_usage->listUsage($image_entity), 'The image has 3 usages.');
...
+      $this->assertIdentical(array('editor' => array('node' => array(1 => '2'))), $file_usage->listUsage($image_entity), 'The image has 2 usages.');
...
+      $this->assertIdentical(array('editor' => array('node' => array(1 => '3'))), $file_usage->listUsage($image_entity), 'The image has 3 usages.');
...
+      $this->assertIdentical(array('editor' => array('node' => array(1 => '2'))), $file_usage->listUsage($image_entity), 'The image has 2 usages.');
...
+      $this->assertIdentical(array('editor' => array('node' => array(1 => '3'))), $file_usage->listUsage($image_entity), 'The image has 3 usages.');
...
+      $this->assertIdentical(array('editor' => array('node' => array(1 => '2'))), $file_usage->listUsage($image_entity), 'The image has 2 usages.');
...
+      $this->assertIdentical(array(), $file_usage->listUsage($image_entity), 'The image has zero usages again.');

Since these assertion messages are in a loop I think we should be specifying which image.

There's only 3 image entities. So each message appears three times. When I was debugging this patch, that wasn't hard to work with at all.

@Wim Leers I dunno repeated test assertion messages that aren't asserting the same thing seems easy to fix and not something we should be encouraging.

Alright.

@alexpott you are right! Here it is.

Thanks!

Committed/pushed to 8.0.x, thanks!