Editor module does not honor file.settings.make_unused_managed_files_temporary

Should probably get a test showing this problem.

@smustgrave I'm not pretty sure about what you mean by getting a test. A Kernel test to prove this would be rather complex, as we would have to mock the EntityRepository and make it return a spy-mock of a File for some uuid.

Besides that, we couldn't directly spy calls to setPermanent() as the setPermanent() method is really not called twice, thanks to a guard that calls first to isPermanent(), but the intention to set the file as permanent is there.

class FileUsageBase {
  ...
  public function add(FileInterface $file, $module, $type, $id, $count = 1) {
    // Make sure that a used file is permanent.
    if (!$file->isPermanent()) {
      $file->setPermanent();
      $file->save();
    }
  ...
}

I've done an "empirical" test and taken a screenshot so the issue can be checked:

As you can see in the last three items in the call stack:

1. First _editor_record_file_usage() is called.
2. Then DatabaseFileUsageBackend::add() is called
3. Finally FileUsageBase::add() is called, where the intention is performed the second time. At this point, isPermanent() already returns TRUE, since it was set by _editor_record_file_usage()

.

Is this enough to check the issue?

NW is more appropriate.

Maybe we can just add a test (if it's not existing yet) that when we create a file using the Editor module, it ends up being permanent on the end. Not specifically checking if setPermanent is called twice, but just the end result.

Also sorry for the commit and not applying the code suggestion, totally missed that.

Ok, I actually think this is already being tested.

https://git.drupalcode.org/issue/drupal-3496195/-/blob/3496195-editor-mo...

That makes sense to me, @bramdriesen. The risk I noticed here lies with the file.usage service and the module editor potentially interfering with each other when setting a file to permanent. It feels like a case of duplicated code.

Ultimately, as for the tests, you're right that the final outcome and the critical functionality are what matter most (and you’ve confirmed those are already being tested).

So, I suppose that if we remove the code and the test EditorFileUsageTest::testFileSaveOperations() doesn’t break, then it should be fine, right? :)

Correct 🙂 a core maintainer can probably verify best if the current test is sufficient. But to me it looks to be the case. Latest test run is 100% green as wel after a random test failure.

So I think we can actually set this as RTBC.

I read the IS, comments and the MR. When removing code, even if it looks wrong, we should check the original issue to see if there is a reason this was done. I have not done that for this issue. The if block being removed was added in #1932652: Add image uploading to WYSIWYGs through editor.module → .

If this change is accepted there is a comment that would need to be changed. I left a comment in the MR.

Updated the IS to mention that this is relying on existing tests.

The subject code in #1932652 was developed on August 2013 (although it was commited later). In that moment, the code setting the file as permanent in FileUsageBase::add() already existed. So I don't really know why this was done twice...

@quietone, additionally I don't see your comment in the MR 10700 about a comment that has to be changed.

So what are the steps to reproduce this bug?

Also as test coverage goes if this is a bug then the current test isn't catching it right.

I forgot to save my comment and when I reread it I am not sure what I was referring to, so I have deleted it.