editor.module should use the same data- attributes as entity_embed.module uses

But then it's no longer just file UUIDs… it's really just "referenced entity UUIDs", right?

Therefore, we might want to rename the data-editor-file-uuid attribute to data-entity-uuid, just like Entity Embed uses, plus a data-entity-type attribute, also just like Entity Embed uses. Then Editor would only register file usages for file entities. And everything would be using the same data- attributes.

But Entity Embed uses a custom tag, right? So there's no potential for confusion over who should render it?

Both solutions should work fine. Using standard data- attributes across all systems would probably bring other benefits also (tracking WYSIWYG usage for all entity types?). Could we do both; change data- attributes and expose an alter hook?

Using standard data- attributes across all systems would probably bring other benefits also

My thoughts exactly!

tracking WYSIWYG usage for all entity types

Yes, except that we only do "file usage" tracking.

This was discussed at DrupalCon Amsterdam with Dave Reid and Devin Carlson. We agreed that it'd be better to change the data- attribute that editor.module uses to the same ones as used by entity_embed, while we still can, before RC.

This will greatly enhance consistency during the Drupal 8 lifecycle and hence also improve usability (no more WTFs about why files embedded by editor.module aren't treated the same as those embedded by entity_embed.module).

Wim Leers just introduced me to this issue.
I'll have a go and will report when I stop working on this.

Made a new patch, based of the conclusion of comment #7.
Replaced data-editor-file-uuid with data-entity-uuid and added data-entity-type.
Also extended the related, already existing automated tests a little bit. They succeed on my local.

Wow, awesome work, PieterDC, thank you very much!

I have only nitpicks :)

1. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -18,7 +18,7 @@
   +      // additional data-entity-type and data-entity-uuid attribute.
   

   s/attribute/attributes/

2. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -57,8 +57,10 @@
   +          // Parse the data-entity-type attribute.
   +          data['data-entity-type'] = element.attributes['data-entity-type'];
   
   @@ -71,7 +73,8 @@
   +          'data-entity-type': 'data-entity-type',
   
   +++ b/core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
   @@ -79,7 +79,8 @@
   +          attrs['data-entity-type'] = this.data['data-entity-type'];
   
   @@ -112,8 +113,10 @@
   +          data['data-entity-type' ] = attrs['data-entity-type'];
   +          delete attrs['data-entity-type'];
   
   +++ b/core/modules/editor/src/Form/EditorImageDialog.php
   @@ -214,7 +214,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $form_state->setValue(array('attributes', 'data-entity-type'), 'file');
   

   Why not just hardcode this?
   No, no! Scratch that! What you did is much better! :) What you did, allows contrib modules to have images that come from entities other than file entities! This is even better!

3. +++ b/core/modules/editor/editor.module
   @@ -482,7 +482,7 @@ function _editor_get_formatted_text_fields(FieldableEntityInterface $entity) {
   - * Parse an HTML snippet for any data-editor-file-uuid attributes.
   + * Parse an HTML snippet for any linked file with data-entity-uuid attributes.
   
   @@ -495,8 +495,8 @@ function _editor_parse_file_uuids($text) {
   +  foreach ($xpath->query('//*[@data-entity-type="file" and @data-entity-uuid]') as $node) {
   +    $uuids[] = $node->getAttribute('data-entity-uuid');
   
   +++ b/core/modules/editor/src/Plugin/Filter/EditorFileReference.php
   @@ -70,12 +70,12 @@ static public function create(ContainerInterface $container, array $configuratio
   -    if (stristr($text, 'data-editor-file-uuid') !== FALSE) {
   +    if (stristr($text, 'data-entity-type="file"') !== FALSE) {
   ...
   -      foreach ($xpath->query('//*[@data-editor-file-uuid]') as $node) {
   -        $uuid = $node->getAttribute('data-editor-file-uuid');
   +      foreach ($xpath->query('//*[@data-entity-type="file" and @data-entity-uuid]') as $node) {
   +        $uuid = $node->getAttribute('data-entity-uuid');
   

   From Dave Reid's writing in the issue summary:

   It would be nice if we could support Editor's tracking of file usage

   Hence we shouldn't restrict this to only handle files, but it should simply handle any entity type.

   In other words: we should indeed require a data-entity-type attribute to present, but we shouldn't require them to have the value 'file'.

4. +++ b/core/modules/editor/src/Tests/EditorFileReferenceFilterTest.php
   @@ -68,45 +68,50 @@ function testEditorFileReferenceFilter() {
   +    $input = '<img src="llama.jpg" data-entity-type="invalid-entity-type-value" data-entity-uuid="' . $uuid . '" />';
   +    $output = $test($input);
   +    $this->assertIdentical($input, $output->getProcessedText());
   

   Yes, an extra test case that is now needed to test this new permutations, excellent! :)

5. +++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
   @@ -90,10 +92,11 @@ public function testEditorEntityHooks() {
   -    $new_value = str_replace('data-editor-file-uuid', 'data-editor-file-uuid-modified', $original_value);
   +    $new_value = str_replace('data-entity-type', 'data-entity-type-modified', $original_value);
   +    $new_value = str_replace('data-entity-uuid', 'data-entity-uuid-modified', $new_value);
   

   Modifying either the data-entity-type attribute or the data-entity-uuid attribute should be sufficient. And we should test both.

   E.g. first modify the data-entity-type attribute, then the number of usages should decrease to 2. Then modify the data-entity-uuid attribute in the older revision, which should further decrease the number of usages to 1.

Thanks for the thorough review.

1. Spelling mistake fixed.
2. No action required.
3. I agree it would be nice to count usage of any entity type instead of just files. But I feel it's out of scope of this issue to do so. Because the code is file oriented. I would rather create a separate issue to provide an EntityUsageInterface etcetera.
4. No action required.
5. Split up the test a bit more. First tried exactly like you suggested, but got into trouble because using entity_revision_load() to load an older revision, you get an object with the default/current revision as 'original' entity. But that's unusable in editor_entity_update(). So, I went for the approach that doesn't require to update older revisions.

• #13.1: Thanks.
• #13.3: That's fair.
• #13.5: Thanks!

Thanks, Pieter! :)

Tagging with D8 upgrade path, since this 1) affects existing D8 sites, 2) would require an upgrade path once Drupal 8 officially supports beta-to-beta upgrades. I'm not sure whether this patch should provide an upgrade path or not, so: dear committer, I defer to your judgement in that area.

Well we need an 8.x to 8.x change record - so people can work out how to upgrade their sites - and know that something has changed.

what would the hook_update_N do?

The hook_update_N would loop over all saved field (?) data in the database and replace all ' data-editor-file-uuid="X"' occurences with ' data-entity-uuid="X" data-entity-type="file"'.

#15: Done: https://www.drupal.org/node/2381651.

#16: #17 is right.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3c6f8b8 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/editor/src/Tests/EditorFileUsageTest.php b/core/modules/editor/src/Tests/EditorFileUsageTest.php
index 3c8587b..3e66cef 100644
--- a/core/modules/editor/src/Tests/EditorFileUsageTest.php
+++ b/core/modules/editor/src/Tests/EditorFileUsageTest.php
@@ -101,7 +101,7 @@ public function testEditorEntityHooks() {
     $this->assertIdentical(array('editor' => array('node' => array(1 => '2'))), $file_usage->listUsage($image), 'The image has 2 usages.');
 
     // Test editor_entity_update(): increment again by creating a new revision:
-    // readd the data- attributes to the body field.
+    // read the data- attributes to the body field.
     $node->setNewRevision(TRUE);
     $node->get('body')->first()->get('value')->setValue($original_value);
     $node->save();
@@ -116,7 +116,7 @@ public function testEditorEntityHooks() {
     $this->assertIdentical(array('editor' => array('node' => array(1 => '2'))), $file_usage->listUsage($image), 'The image has 2 usages.');
 
     // Test hook_entity_update(): increment, by modifying the last revision:
-    // readd the data- attributes to the body field.
+    // read the data- attributes to the body field.
     $node->get('body')->first()->get('value')->setValue($original_value);
     $node->save();
     $this->assertIdentical(array('editor' => array('node' => array(1 => '3'))), $file_usage->listUsage($image), 'The image has 3 usages.');

Correct the spelling mistakes.

Looks like this broke the widget : #2381719: Image CKE widget is lost on submit, plain img instead

Actually, looks like we were both bitten by aggressive caching in the browser :)

Hm, yeah, looks like weird browser cache behavior, sorry about that.

Looking at the code that got in, though :

+++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
@@ -40,14 +40,14 @@
// In downcast function:
-          element.attributes['data-editor-file-uuid'] = this.data['data-editor-file-uuid'];
+          element.attributes['data-entity-uuid'] = this.data['data-entity-uuid'];

@@ -57,8 +57,10 @@
// In upcast function:
-          // Parse the data-editor-file-uuid attribute.
-          data['data-editor-file-uuid'] = element.attributes['data-editor-file-uuid'];
+          // Parse the data-entity-type attribute.
+          data['data-entity-type'] = element.attributes['data-entity-type'];
+          // Parse the data-entity-uuid attribute.
+          data['data-entity-uuid'] = element.attributes['data-entity-uuid'];

Looks weird that upcast does stuff about data-entity-type & data-entity-uuid, but downcast only places the latter ? When would data-entity-type ever be present then ?