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.
I just noticed that images inserted ONLY to teasers disappeared from my site on the other day. I am not sure but i think it is a bad behavior for the orphaned files. May somebody wants to insert a smaller picture for a small teaser for the actual node.
I disabled orphaned file removing in the file system options as workaround and i will check it later.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2725415.patch | 2.35 KB | catch |
#13 | 2725415-13.patch | 2.39 KB | catch |
#13 | 2725415-tests.patch | 1.77 KB | catch |
#10 | 2725415-10.patch | 1.78 KB | catch |
#10 | 2725415-test.patch | 1.16 KB | catch |
Comments
Comment #2
swentel CreditAttribution: swentel commentedSounds a bit like what happens in #2683679: Images incorrectly deleted too - still hard to reproduce the behavior.
Comment #3
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedHi @juhaszg,
Are you inserting images to teasers this way?
Comment #4
alexpottComment #5
Berdir\_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.
Comment #6
catchToo late to kill the field type in 8.x, but I opened #2726497: Deprecate text_with_summary for things we can do.
Comment #7
Wim LeersUGHHHHHHHHHHH.
<3 Berdir
<3 catch
Comment #8
Wim LeersClosed #2683679: Images incorrectly deleted as a duplicate.
Comment #9
catchPatch itself looks fine, marking NW for tests.
Comment #10
catchHere's those tests.
Comment #12
Wim LeersHm… this makes me think that we should actually also test the case of an image being embedded in both
value
andsummary
. They should all be counted.Comment #13
catchSo 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.
Comment #14
Wim LeersIsn'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.
Comment #17
catchDiscussed this with @alexpott @xjm @effulgentsia @cotsser and @webchick and we agreed it's clear data loss and definitely critical.
Comment #18
catch@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.
Comment #20
catchComment #22
catchMigrate random fails. Back to CNR - got a green test run on the 8.2.x patch at least.
Comment #23
Wim LeersComment #25
Wim LeersThe random migrate fail in 8.1.x keeps reoccurring. Re-tested yet another time.
Comment #27
Wim LeersIt is finally actually green.
Comment #28
xjmI'm concerned about the 8.1.x fail; this does not look like a Migrate fail:
I sent 8.2.x for a retest.
Comment #29
xjmAlright, 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").
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?
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.
Comment #30
xjm@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.
Comment #33
xjmI 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!
Comment #34
Wim LeersD'oh, you're right :( I totally forgot about that. Sorry. Thanks for catching my mistake.
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 likevalue
andsummary
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.
Comment #35
Wim LeersRegarding the need for a follow-up:
Let's look at
_editor_get_formatted_text_fields()
: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
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):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
tag.Comment #36
BerdirOne final remark
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?
Comment #37
Wim Leers+1.
The only way we can possibly fix this, is by having richer metadata in Typed Data's "data definitions".
Comment #38
BerdirCreated #2732429: Add more metadata to field type properties to identify text with a format and separate user provided text from machine names
Comment #39
catch#38 looks dead on for follow-ing up on the hardcoding.