editor.module causes fatal error for images with invalid or non-existing file UUIDs

+++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
@@ -68,7 +68,7 @@ public function testEditorEntityHooks() {
       'title' => 'test',
       'body' => array(
-        'value' => '<p>Hello, world!</p><img src="awesome-llama.jpg" data-editor-file-uuid="' . $image->uuid() . '" />',
+        'value' => '<p>Hello, world!</p><img src="awesome-llama.jpg" data-editor-file-uuid="' . $image->uuid() . '" /><img src="awesome-llama.jpg" data-editor-file-uuid="invalid-editor-file-uuid-value" />',
         'format' => 'filtered_html',
       ),
       'uid' => 1,

Some comment above would be great to explain what we are testing here.

Great!

+++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
@@ -103,6 +103,11 @@ public function testEditorEntityHooks() {
+    // Test hook_entity_update() by adding an invalid data- attribute.
+    $node->get('body')->first()->get('value')->setValue($original_value . '<img src="awesome-llama.jpg" data-editor-file-uuid="invalid-file-uuid" />');
+    $node->save();
+    $this->assertIdentical(array('editor' => array('node' => array(1 => '2'))), $file_usage->listUsage($image), 'The image still has 2 usages.');

I wonder whether we could also test a non-existing UUID

So very close! I'm sure the next one will be RTBC :)

There should also be test coverage for hook_entity_insert() and hook_entity_delete(). Finally, ideally this would test both an invalid UUID (like you're doing now) and a non-existing UUID.

+++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
@@ -103,6 +103,11 @@ public function testEditorEntityHooks() {
+    // Test hook_entity_update() by adding an invalid data- attribute.

This should be Test hook_entity_update()'s handling of an invalid data- attribute.

This should be Test hook_entity_update()'s handling of an invalid data- attribute.

What about directly pointing to the code handling that?

Awesome!

+++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
@@ -103,6 +103,16 @@ public function testEditorEntityHooks() {
+    // Test _editor_delete_file_usage()'s handling of invalid data- attributes.
+    $node->get('body')->first()->get('value')->setValue($original_value);
+    $node->save();
+    $this->assertIdentical(array('editor' => array('node' => array(1 => '2'))), $file_usage->listUsage($image), 'The image has 2 usages.');

$original_value contains a valid file entity UUID. So this doesn't test the deletion hook in any way, it tests the insertion hook for a valid UUID, which is a case already covered by existing tests.

Your assertion also proves this: the assertion verifies that after this code has run, there are two usages, versus one before. If this was the deletion of an invalid file UUID, then the number of usages would have to remain constant.

Therefore, I'm afraid that #9 is wrong. It claims to test hook_entity_delete(), but it doesn't, as I've explained above. It also doesn't yet test hook_entity_insert(). And it also doesn't do this yet:

Finally, ideally this would test both an invalid UUID (like you're doing now) and a non-existing UUID.

So… I'm afraid I have to unRTBC this :(

Do you think it is necessary to run those again with invalid values? If so, maybe just append them to original value like in #0 editor_file_usage-fail.patch […]

Yes and yes :)

About the UUIDs, the latter is just a random one and the other value is what you get if you disable file uploads. Which values would you prefer?

Both! A valid, but non-existing UUID and something that doesn't even resemble a UUID.

Thank you again and sorry for not providing interdiffs!

Thank you, and no worries! :)

Perfect — thank you :)

Committed 7e2ab24 and pushed to 8.0.x. Thanks!