Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
A module providing a field type can use the TextFormat element type to invoke editor handling. However, doing so will lead to the bug that files embedded vie an editor in the text area will not be registered as used and therefore, if not used elsewhere, by default will be deleted.
This bug occurs for example in the FAQ Field module, see #2744905: Inline images in the answer field are removed during file garbage collection.
The issue arises from the function _editor_get_formatted_text_fields(), which is used to determine whether a given field should be scanned for embedded files. The function uses a hard-coded list of field types:
// 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);
}));
Proposed resolution
Criteria to consider for a solution
- Has to support the three existing core field types, 'text', 'text_long', 'text_with_summary'.
- Supported field types must have a
valueproperty, since the processing in_editor_get_file_uuids_by_field(), which calls_editor_get_formatted_text_fields(), consults$field_item->value.
Proposed solution
Examine field types and return those that are a subclass of TextItemBase.
This is an interim improvement pending a fuller solution in #2732429: Add more metadata to field type properties to identify text with a format and separate user provided text from machine names.
Pros:
- Reduces the hard-coding/special-casing of core field types.
- Maintains support for the three existing core field types.
- Encourages other modules providing formatted text field types to extend the existing core ones or a common base, rather than adopting custom solutions.
- Avoids significant API changes.
Cons:
- May be difficult for module developers to discover. Would need documentation.
- Doesn't fully remove special-casing of core field types, since this snippet remains:
if ($field_item->getFieldDefinition()->getType() == 'text_with_summary') { $text .= $field_item->summary; }However, that could be separately addressed (for example, by determining if the given field type plugin has a 'summary' property instead of using a hard-coded field type ID).
Remaining tasks
User interface changes
None.
API changes
Modules providing a field type that includes filtered text should extend TextItemBase or one if its subclasses so that file usage of embedded files will be handled by the Editor module.
An example of such a field type plugin is the Editor Test module's Drupal\editor_test\Plugin\Field\FieldType\EditorTestTextLongItem class.
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #38 | 2857444-8.9.x-38.patch | 5.82 KB | alexpott |
| #33 | interdiff.txt | 2.9 KB | nedjo |
| #33 | editor-field-types-2857444-33.patch | 5.82 KB | nedjo |
Comments
Comment #2
nedjoPatch using the first of the approaches in "Proposed resolution".
Comment #4
nedjoComment #5
nedjoThis doesn't turn out to be a great approach because the code in Editor assumes a 'value' property, which field types may or may not define.
This might be a better alternative.
Comment #6
nedjoTesting instead for subclass of
TextItemBase.Comment #7
nedjoComment #8
jofitz CreditAttribution: jofitz at ComputerMinds commentedNo longer applies after https://www.drupal.org/node/2776975.
Comment #9
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #10
Wim LeersThe proper solution is #2732429: Add more metadata to field type properties to identify text with a format and separate user provided text from machine names. It may not be a practical solution though.
Also this is not a bug, this is a missing capability.
Comment #11
nedjoTrue of the title I gave the issue, but there is a bug here.
If the editor didn't attach to non-core text fields, or didn't allow files to be embedded in non-core fields, this would not be a core bug. But it does. #2744905: Inline images in the answer field are removed during file garbage collection is a bug, and it results not from misuse of a core API but from using the API as designed.
So changing title to clarify the bug, which is parallel to #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations and #2725415: Text Editor module fails to track usage of images uploaded in text_with_summary fields, allows uploaded images to be deleted.
Agreed that #2732429: Add more metadata to field type properties to identify text with a format and separate user provided text from machine names would be a better solution. But given that #2732429 seems far from a resolution, is the limited improvement in this issue worth considering?
Comment #12
Wim LeersAbsolutely!
On two conditions:
@todoexplaining that this must be removed as soon as #2732429 landsTextItemBaseThen I'd be happy to RTBC this :) This is a totally solid, totally not-BC-nightmare-causing, practical temporary work-around.
(I'm sorry that I implied otherwise — I was just in a hurry.)
Comment #13
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedAdded @todo.
Comment #14
nedjoNo prob. Thanks for the closer look.
@todo.Comment #17
nedjoOkay, apparently there's a reason the test module config is added programmatically, so here's a patch that doesn't mess with that ;)
Comment #19
nedjoUpdating summary with API changes.
Comment #22
scottsawyerI ran into an issue with the markup module that seems to be related, after some period of time ( probably after cron ), inline files uploaded through the editor attached to a Markup field are removed.
I filed this issue in that queue: https://www.drupal.org/project/markup/issues/2943406
I wanted to find out if this patch works in Drupal 8.4, and if the issue I referenced is likely due to the same bug.
If it is the same bug, what, if anything, does changes to that module would need to be made to ensure files uploaded don't get removed?
Comment #23
nedjoSee the API changes section of the issue summary.
Comment #24
vaccinemedia CreditAttribution: vaccinemedia at CTI Digital commentedI'm having this exact issue on the simple block module @scottsawyer and there is a related issue here: https://www.drupal.org/project/simple_block/issues/2961654 - and the comment makes sense where they mention exporting the block contents to config wouldn't export the image file.
Comment #27
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, I'm experiencing the same bug on body fields :-(
Comment #29
hgneng CreditAttribution: hgneng commentedThis is how I fix this issue right now. I detect images in text_format content and add file usage for them after form submit.
https://cto.eguidedog.net/node/997
Comment #32
pcambraI think this has tests now.
I think it could be good to have some sort of property in the custom fields that says "I might have an image, please don't delete it", but I've tested the patch on 9.1.3 and works for my use case, the fields need to inherit from TextItemBase and they need to be able to expose the "value" property. Any other combination will result in mysteriously missing files so that grants, I think a priority bump.
Comment #33
nedjo@pcambra thx for picking up this old issue.
Rebased on 9.2.x and made three small changes.
This should be 'editor_test_text_long' rather than 'text_long'.
The form of the second argument to
is_subclass_of()is inconsistent in core. Maybe in general in core we're moving towards a use statement andClassName::classrather than passing the full class name as a string? Changing in any case.The other parallel calls in the test have been changed to
::assertSame()so presumably we should do the same.Comment #34
alexpottThis looks like a sensible fix to a problem the would result in data loss.
Committed and pushed 02388c80eb to 9.2.x and 21af6a87ad to 9.1.x. Thanks!
Comment #37
alexpottComment #38
alexpottDiscussed with @catch we agreed to backport this to 8.9.x if a patch can be rolled based on #33.
Git can auto-merge this so here's the patch...
Comment #39
florianmuellerCHWe have a custom config form with multiple long text (formatted) fields. When uploading images there and saving, the usage is still not registered with this patch. Any Idea how this could be solved?
Comment #40
alexpott@fmueller_previon I think this is because you're saving things in config and not an entity field. This is about entity fields.
Comment #41
florianmuellerCH@alexpott that's correct, but in this case we don't have a solution for this yet.
I ended up parsing the text for any file entities and then manually adding a File Usage on a certain Entity (I attached them to the frontpage), just to ensure they won't get deleted. I'm not quite sure if this is "the way to go" for that ;)
Comment #42
alexpottAs per the discussion with @catch backported to 8.9.x.
Committed 16d35ec and pushed to 8.9.x. Thanks!
@fmueller_previon I think the discussion about config fields deserves it's own issue as currently Drupal core make no attempt to count files used in config.
Comment #45
hatuhay CreditAttribution: hatuhay commentedCannot reopen but this is not fixed on D8.9.15 and patch fail to apply on this version
Comment #46
galactus86 CreditAttribution: galactus86 as a volunteer and at Mediacurrent commentedany update on this?
Comment #47
mashot7Having the same issue for 9.3.21
Comment #48
nedjoNote that this issue did not in itself fix existing issues in contrib modules. Instead, it provided an API method contrib modules could choose to use, see the API changes section of the issue summary.
If you're seeing a bug like that described in this issue you can look for an existing issue on the contrib module you're using and, if you don't find one, open a new one, referencing this issue.