Text Editor module fails to track usage of images uploaded in text_with_summary fields, allows uploaded images to be deleted

Sounds a bit like what happens in #2683679: Images incorrectly deleted too - still hard to reproduce the behavior.

Hi @juhaszg,

Are you inserting images to teasers this way?

1. Add content, e.g.: node/add/page
2. Upload the image to the body field via WYSIWYG.
3. Copy the URL of the image to the clipboard.
4. Click the 'Edit summary' link.
5. Create an image tag and pasting in the URL.
6. Click on the image in the WYSIWYG and press "delete"
7. Save the content.

\_editor_get_file_uuids_by_field() hardcodes the ->value property for the property containing "the text".

Instead, it should look at everything. We have no way of knowing what is actually text and what is not, so we'll end up parsing e.g. the format as well, or target_ids.

However, looking at _editor_get_formatted_text_fields(), we hardcode the field types we support anyway, so one more hardcoded check doesn't hurt more.

Still needs tests, we might also need an update function but that will be fun. It would be somewhat similar to a function we wrote in paragraphs, where we loop over all fields of a given type and then all entities that have a value in those fields. We could also say that existing files are probably already gone and there's not much we can do, unless users actually disabled deleting unused files.

I suppose the discussion around text_with_summary will also come up again and yes, we should have removed that a long time ago and simply have two fields but it's way too late for that for 8.x.

I manually tested the attached patch, worked.

Too late to kill the field type in 8.x, but I opened #2726497: Deprecate text_with_summary for things we can do.

UGHHHHHHHHHHH.

<3 Berdir

<3 catch

Closed #2683679: Images incorrectly deleted as a duplicate.

Patch itself looks fine, marking NW for tests.

Here's those tests.

+++ b/core/modules/editor/tests/src/Kernel/EditorFileUsageTest.php
@@ -177,6 +177,17 @@ public function testEditorEntityHooks() {
+    // Empty out the body value, but populate the summary instead. Usage should
+    // not change.
+    foreach ($original_values as $key => $original_value) {
+      $node->body[$key]->value = '';
+      $node->body[$key]->summary = $original_value;
+    }

Hm… this makes me think that we should actually also test the case of an image being embedded in both value and summary. They should all be counted.

So I agree we should test it, but because it's the same file referenced by the same node and revision, it only results in the same number of usages as if one of them has the reference.

Here's that coverage added though.

it only results in the same number of usages as if one of them has the reference.

Isn't that a bug? Albeit a pre-existing bug, of course. If you have a file field, and it references the same file twice, then you have two usages, right?

#2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations contains a fix for this. But probably it should not be fixed here, nor in that issue, but in a separate issue.

Discussed this with @alexpott @xjm @effulgentsia @cotsser and @webchick and we agreed it's clear data loss and definitely critical.

@Wim I'm not sure if it's a bug or not. When we save the entity, we examine all fields, so it's a question of whether we track the entity -> file reference or the field -> file reference. However we should pick one, stick to it and document it. Opened #2729953: Clarify handling of multiple references to the same file from entities.

Here's a re-roll.

Migrate random fails. Back to CNR - got a green test run on the 8.2.x patch at least.

The random migrate fail in 8.1.x keeps reoccurring. Re-tested yet another time.

It is finally actually green.

I'm concerned about the 8.1.x fail; this does not look like a Migrate fail:

	Views.Drupal\Tests\views\Kernel\Handler\FilterEqualityTest
✗	
rupal\Tests\views\Kernel\Handler\FilterEqualityTe
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1943.xml:
PHPunit Test failed to complete
✓		- testEqual
✓		- testEqualGroupedExposed
✗	
testNotEqual
fail: [Other] Line 85 of core/modules/views/tests/src/Kernel/Handler/FilterEqualityTest.php:
Drupal\Tests\views\Kernel\Handler\FilterEqualityTest::testNotEqual
Drupal\Core\Database\SchemaObjectExistsException: Table sequences already exists.

/var/www/html/core/lib/Drupal/Core/Database/Schema.php:592
/var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:806
/var/www/html/core/modules/views/tests/src/Kernel/ViewsKernelTestBase.php:44
✓		- testEqualGroupedNotExposed
✗	
nkno
fail: [run-tests.sh check] Line 0 of :
FATAL Drupal\Tests\views\Kernel\Handler\FilterEqualityTest: test runner returned a non-zero error code (2).

I sent 8.2.x for a retest.

Alright, I ran the failing test locally a few dozen times and convinced myself it was just a bot environment cleanup problem ("Table sequences already exists").

Instead, it should look at everything. We have no way of knowing what is actually text and what is not, so we'll end up parsing e.g. the format as well, or target_ids.

However, looking at _editor_get_formatted_text_fields(), we hardcode the field types we support anyway, so one more hardcoded check doesn't hurt more.

Yeah, I think this is fine as a hotfix for those reasons. Are there any followups we can do in Editor for a more robust solution?

we might also need an update function but that will be fun. It would be somewhat similar to a function we wrote in paragraphs, where we loop over all fields of a given type and then all entities that have a value in those fields. We could also say that existing files are probably already gone and there's not much we can do, unless users actually disabled deleting unused files.

Looks like this suggestion has not been addressed? What would we be updating? If we delivered this hotfix in 8.1.2 but decided to roll out an upgrade path afterward, what impact would that have? It seems to me we could definitely save at least some data, but OTOH we would be losing more if we miss 8.1.2 writing a costly update.

Bumping back to NR for those questions. I'm okay committing it without an update function because the sooner we release this the better; just want to make sure we address it if we do have the chance.

@catch and I discussed this and decided we could explore the possibility of an update function (or how to handle it) in a followup. Through the UI, it is possible to set files to be purged up to "3 months" after deletion (in addition to "never"), so there is definitely the possibility to save some data.

Also, note that this is not reproducible on a default standard install on 8.2.x, because apparently revisions have the opposite bug: file usage associated with revisions isn't always updated (even if the entire entity is deleted): #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger.

I was able to reproduce the bug on 8.1.x standard by hacking in an option to delete orphaned files after 1 second, and to confirm that the patch resolves it without introducing any additional stale files for non-revisioned entities.

I posted: #2732393: Decide if and how to try to save incorrectly orphaned images uploaded in text_with_summary fields from #2725415 Leaving as "Needs followup" for the potential discussion of a more general improvement to how Editor tracks usage data, but I think this is sufficient to commit the hotfix.

Committed and pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Looks like this suggestion has not been addressed?

D'oh, you're right :( I totally forgot about that. Sorry. Thanks for catching my mistake.

Are there any followups we can do in Editor for a more robust solution?

Not yet, because it looked like there can't be. The best we can do is knowing whether a certain property contains a string, which \Drupal\text\Plugin\Field\FieldType\TextWithSummaryItem::propertyDefinitions() can tell us. But even then, format is a string just like value and summary are, so they don't help.
Note that \Drupal\text\Plugin\Field\FieldFormatter\TextTrimmedFormatter::viewElements() also just hardcodes which properties it should deal with. This is simply an unsolved problem in Entity API/Field API/Typed Data API.
Also: #2726497: Deprecate text_with_summary.

Regarding the need for a follow-up:

However, looking at _editor_get_formatted_text_fields(), we hardcode the field types we support anyway, so one more hardcoded check doesn't hurt more.

Let's look at _editor_get_formatted_text_fields():

/**
 * Determines the formatted text fields on an entity.
 *
 * @param \Drupal\Core\Entity\FieldableEntityInterface $entity
 *   An entity whose fields to analyze.
 *
 * @return array
 *   The names of the fields on this entity that support formatted text.
 */
function _editor_get_formatted_text_fields(FieldableEntityInterface $entity) {
  $field_definitions = $entity->getFieldDefinitions();
  if (empty($field_definitions)) {
    return array();
  }

  // Only return formatted text fields.
  return array_keys(array_filter($field_definitions, function (FieldDefinitionInterface $definition) {
    return in_array($definition->getType(), array('text', 'text_long', 'text_with_summary'), TRUE);
  }));
}

Why do we do this? For the exact reasons stated in #34: Typed Data does not allow us to know which fields would contain formatted text.

Before #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI, we used to do

  // Only return fields that have text processing enabled.
  return array_keys(array_filter($field_definitions, function ($definition) {
    return $definition->getSetting('text_processing') === TRUE;
  }));

But since then, we don't have that setting anymore. Which means we need this metadata to live at the Typed Data level, which it does not. I'm not even sure it can.

And if we go back further, the original name of this function was _editor_get_processed_text_fields(), and it was introduced by #1932652: Add image uploading to WYSIWYGs through editor.module. Quoting #1932652-38: Add image uploading to WYSIWYGs through editor.module, from August 9, 2013 (almost 3 years ago):

Remaining flaws

1. _editor_get_processed_text_fields() essentially hardcodes text.module's field types. I'm not sure how we can get around that. I can think of work-arounds that will enable Drupal 8 contrib to inject more field types, but I cannot think of a way to have them being found automatically; the entity/field system metadata is not rich enough for that AFAIK. In the grand scheme of things, I think this is only a small flaw though, and one that is not caused by or inherent to this issue.

After that, it was not brought up a single time anymore in that issue, not in any of the 73 subsequent comments. Simply because we don't have the metadata to do better.

Given all this evidence, removing the Needs followup tag.

One final remark

Note that \Drupal\text\Plugin\Field\FieldFormatter\TextTrimmedFormatter::viewElements() also just hardcodes which properties it should deal with. This is simply an unsolved problem in Entity API/Field API/Typed Data API.

TextTrimmedFormatter has a hardcoded list of field types it supports, it can never be used for something else (Unless someone alters and extends the definition).

That's different for editor.module, it doesn't know for sure on which types it is used, as it is tied to form elements, not field types. So not really the same case.

In TMGMT, I have a similar but not identical problem. Given an entity with field definitions, I want to extract all translatable text from it. That can be text fields but also string and other field types. (but not list_string, as the values there are machine names, basically, similar to format). Additionally, and that's more related to the problems that editor.module has, I would have to know how format/value/summary are connected and for which properties a format is. See http://cgit.drupalcode.org/tmgmt/tree/sources/content/src/Plugin/tmgmt/S...

Maybe we can open an issue for that to think about possible solutions?

Maybe we can open an issue for that to think about possible solutions?

+1.

The only way we can possibly fix this, is by having richer metadata in Typed Data's "data definitions".

Created #2732429: Add more metadata to field type properties to identify text with a format and separate user provided text from machine names

#38 looks dead on for follow-ing up on the hardcoding.