Allow to reference permanent files with 0 usages

Nice find!

Users not being able to save nodes sounds critical to me, even if it only happens with certain modules or on certain sites, so promoting this to critical.

I'll work on the test coverage

Consider the difference between

a) An image disappears on the frontpage, without any user interaction before
b) Someone cannot save a node with a node, which had an file deleted.

Unlike a), b) at least doesn't leave a utterly broken site. B) is also an active process, and as such MUCH easier to figure out. B) also already existed before the other issue.

Given that I still don't understand why this patch was reverted.

Damnit crosspost.

Berdir's test coverage is much better!

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -406,7 +406,7 @@ public static function validateManagedFile(&$element, FormStateInterface $form_s
-          if ($file->isPermanent()) {
+          if ($file->isPermanent() && \Drupal::config('file.settings')->get('make_unused_managed_files_temporary')) {

We need some form of documentation here ...

Referring to #6

I have the smallest of changes .. which can wait until the next iteration.

Some of the new test code is using t('Save') - I think we should simplify those to 'Save'

There is no need to invoke translation in tests unless directly testing translation.

Should be back to green w/ feedback addressed.

We need to mention this issue in the release notes alongside #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary until a fix is committed here.

Will try to post an update tonight.

I read this several times, and think @Berdir's explanation makes sense after the moved comment; just fixed a type in his text. Also added a @see to the CR (which is still unpublished).

Yup. Must have chopped it off when flowing to 80 cols.

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -398,15 +398,23 @@ public static function preRenderManagedFile($element) {
+          // If referencing an existing file, only allow if there are existing
+          // references. This prevents unmanaged files from being deleted if
+          // this item were to be deleted. When files that are no longer in use
+          // are automatically marked as temporary (now disabled by default),
+          // it is not safe to reference a permanent file without usage. Adding
+          // a usage and then later on removing it again would delete the file,
+          // but it is unknown if and where it is currently referenced. However,
+          // when files are not marked temporary (and then removed)
+          // automatically, it is safe to add and remove usages, as it would
+          // simply return to the current state.

Just some random rambling: Sometimes it feels like its easier to express these kind of technical problems using some kind of ascii graph / description of possible states instead of a long text like this. Anyway, this is just a sitenote. This text is totally understandable

1. Sometimes it feels like its easier to express these kind of technical problems using some kind of ascii graph / description of possible states instead of a long text like this.

   Well, this assumes that the person reading the code has the facility to interpret the ascii spatially (sight, spatial reasoning, possibly LTR native language...). For something like a list, the API module can render it semantically, but not if the spaces are a spatial map. In those cases we would at least want the information to be available as parseable text as well.

   In this particular case a list or numbered list could do the trick? The text is good but walls-of-text are best broken up into smaller chunks for readability, generally.

2. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -398,22 +398,23 @@ public static function preRenderManagedFile($element) {
   +          if ($file->isPermanent() && \Drupal::config('file.settings')->get('make_unused_managed_files_temporary')) {
   

   \Drupal::config() here makes it backport-safe. Do we have a followup to add the service?

3. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -398,15 +398,23 @@ public static function preRenderManagedFile($element) {
   +          // @see https://www.drupal.org/node/2891902
   

   Thank you for referencing a CR instead of an issue ID.

Neither of 1-2 is particularly worth blocking commit on given the severity of this issue, so leaving RTBC for the moment.

Normally I might not credit the reasonably small review in #14 but this is definitely something that we want to fix in the long term. Good spot, thanks @martin107!

Going ahead and committing this with the "honor system" for followup(s), given that it would be good to have this in the beta for testing with the file usage change.

We might want to backport this to 8.3.x along with #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary -- the two together might be worth a final 8.3.x patch release. However, that patch should be combined with this one if so.

Is anything really needed for an 8.3.x version? It applies cleanly (I checked), so we can just cherry-pick, depending on what we do with #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary .

And re #26-2, since preRenderManagedFile() is a static method, we can't use DI. Or did you have a different idea? (and thanks to @Sam152 and @ larowlan for reminding me about this).

@mpdonadio, we might as well include it WITH the 8.3.x version (see the other issue for details). They are for release together and the other isn't backported yet.