Files renamed by \_file_save_upload_single() do not have the correct filename on the File entity

FWIW, #2940383: Unify file upload logic of REST and JSON:API currently does not have a goal to share with the UI, I explicitly asked about that in regards to naming. But maybe it should :)

Seems sensible 👍

Note: this will disrupt some contrib modules: those that have functional tests involving file uploads may need to be updated.

Yup, looks good to me, too. It will be nice to consistently have the file entity reflect reality.

Do we need a CR for this?

Otherwise, RTBC from me.

Pretty sure the bot is confused. Requeued #10 for 8.6.x and 8.7.x.

Sad confused bot. :/ Let's try a new test-only from #10 and a re-upload of identically #10.

That's better. Back to RTBC.

I always thought it is by design that we don't rename the filename as that is the name that the user gave it and the renamings that we are doing so far are just for technical reasons. That is changing with the event and more renaming-reasons we are introducing, so I can see why this makes sense.

It does make me wonder whether we need to have a dedicated field for that at all, but entity types without a label field are... icky, e.g. the user entity, which needs custom logic to handle autocomplete and so on.

@Berdir, I am sorry I don't follow your comment in #18. Can you clarify.

#3035320: Random fails in TimestampAgoFormatterTest and DateTimeTimeAgoFormatterTest

Can we get a change record on the basis of #6

@alexpott added https://www.drupal.org/node/3035507

Committed 9431a1c and pushed to 8.7.x. Thanks!

c/p as 3211ad36c2 and pushed to 8.6.x

Published the change record.

This commit was brought to you by 'we're not alone' by dinosaur jr 🎶

This commit was brought to you by 'we're not alone' by dinosaur jr 🎶

😆

@Wim Leers

Note: this will disrupt some contrib modules: those that have functional tests involving file uploads may need to be updated.

Just hit this in Behat tests :)

We just discovered that this change affects the D8 version of upload_replace, which lives in 2868450 as a patch and https://github.com/cgmonroe/upload_replace as a module (until the patch is committed by maintainers).

Since this change updates the filename before upload_replace_file_update() (an implementation of hook_ENTITY_TYPE_update()) sees the original filename, the following check in upload_replace.module is no longer valid:

+    //If the filename has be modified by adding a _0 value, or
+    //on certain situations the filepath will not match the filepath in the db, such as
+    //when reverting a revision.  When reverting a revision change the filename as well
+    if (!strpos($new_file->getFileUri(), $new_file->getFileUri())) {
+      //the filename is not in the filepath, so drupal must have added a "_0" before the extension
+      //find the file that is blocking this file from keeping the correct path

I previously thought upload_replace could instead rely on the fid once 2241865 is committed, but that wouldn't make a difference since _file_save_upload_single is called with FILE_EXISTS_RENAME.

So it looks like this change means that the original filename that the browser sent is lost and therefore when displaying the file link it gets the _0/_1 appended? I note that the description of FileInterface->getFilename() says:

This may differ from the basename of the URI if the file is renamed to
avoid overwriting an existing file.

So I don't think this property was intended to reflect the actual path to the file, that's what the URI is for?

In practical terms, it means that when a user uploads a file with the same name to a bunch of different nodes it displays a link with the suffix included, which isn't what I would expect (and looks bad). If we didn't want to revert the change, could we store the original filename as another property?

@henokmikre @richard.thomas, If bugs come from this issue please open new issues about them and reference this one. No one will see your comments here and nothing will be resolved if there is a problem.

I am able to get the existing filename by using hook_entity_presave and in this function getting the filename by loading the node entity.

Very happy to see this has been fixed! This was always a major point of confusion having a filename property that wasn't actually representative of the actual filename.

I do agree with @Berdir in his comment #18.
There needs to be a way to get the original file's name before it was automatically renamed, as that was the name the user gave it before uploading it, and might be important to preserve.
Ideally, I think this should be saved in an "original_filename" column in the file_managed table.