Follow-up to #2725415: Text Editor module fails to track usage of images uploaded in text_with_summary fields, allows uploaded images to be deleted. That issue hotfixes the Editor module to record usages from files that are used only in the summary for a longtext with summary field. However, it does not provide any update function to restore issues that are already potentially orphaned but not yet deleted.
Such an update would potentially be massive (re-parsing every text with summary field for the files attached in the summary only, basically, and updating their usage data).
Note that it's important to test this on 8.1.x and/or on a non-revisioned entity type because of a different bug: #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger
Also, adding an option to delete files more quickly makes it easier to test manually:
diff --git a/core/modules/system/src/Form/FileSystemForm.php b/core/modules/system/src/Form/FileSystemForm.php
index 9271bf4..6dd403f 100644
--- a/core/modules/system/src/Form/FileSystemForm.php
+++ b/core/modules/system/src/Form/FileSystemForm.php
@@ -120,7 +120,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
);
}
- $intervals = array(0, 21600, 43200, 86400, 604800, 2419200, 7776000);
+ $intervals = array(0, 1, 21600, 43200, 86400, 604800, 2419200, 7776000);
$period = array_combine($intervals, array_map(array($this->dateFormatter, 'formatInterval'), $intervals));
$period[0] = t('Never');
$form['temporary_maximum_age'] = array(Decide whether to provide an update for this, either as a normal required update or some optional thing.
TBD
None.
TBD
TBD
Comments
Comment #2
xjmTentatively setting to critical, because this is still about reducing data loss (even if we decide not to do anything).
Comment #3
wim leersQuoting what I wrote at #2725415: Text Editor module fails to track usage of images uploaded in text_with_summary fields, allows uploaded images to be deleted, because I cross-posted that with you opening this issue:
Comment #4
xjm@Wim Leers, wrong issue I think; this one is only about whether we provide an update function to try and recover files on existing sites that are marked "Temporary" but not yet purged following the critical bugfix.
Comment #5
catchI think we can provide a drush command, or a special (experimental?) module with a batch page that does this either in core or contrib. However I really wouldn't want to put it in an update, since it will essentially mean putting sites with lots of content into maintenance mode for potentially hours or days, in some (nearly all?) cases for no purpose at all. Or we'd need to add an API and user interface for 'choose your own adventure' updates and make it one of those.
Comment #6
catchAlso what we'd be doing is "load and resave every entity", or possibly "load and resave every entity with a text+summary field. That's a common need, so we should think about generalising it.
The more I think about this, the more running this as an update on every site to fix potential data loss via an edge case feels more harmful to sites overall than not doing it, and if we don't run it for every site, we can't guarantee fixing the data on the sites that might have it (since they have no way to know if they're affected or not). So I'd drop this one out of the critical queue and move it to major task. It's also mitigated a bit by #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger in a strange case of two wrongs making a right.
Comment #7
wim leershahahah :D
… even more so because in most cases the harm will already have been done: the uploaded file will already have been deleted by now. If a file is already gone, then
\file_cron()will also have deleted the file entity, at which point we literally cannot fix it retroactively anyway.Comment #8
xjmSince #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger also has a potential need to optionally rebuild the usage data, maybe we could come up with a shared solution.
Well, this depends on the site's configuration. The default time to purge is only 6h, but at a local event people were using both "3 months" and "never" which can be configured via the UI.
I can see the case for downgrading it also, but I'm on the fence. If it were my site I would darn well want Drupal to save as many of my images as possible.
Comment #9
xjmI will say having a way to rebuild the data (independently of the criticals) seems like a good idea anyway, since the current behavior is a bit brittle.
Comment #10
xjm@catch, @effulgentsia, @alexpott, @Cottser, and I discussed this issue today and agreed that is probably a wontfix, because a required update on the off chance we could retrieve some unpurged files is not feasible, and an optional one is increasingly less useful since by default all this data would have already been purged.