MediaObserver updating fails if media file_name changes but path doesn't

[SlyDave]

As noted in the discussion #3089 , if you set moves_media_on_update to true, and then update a media's file_name, but its generated path doesn't change the MediaObserver exceptions with:

League\Flysystem\FileExistsException
File already exists at path: <path>

on line 29 of the MediaObserver

        if (config('media-library.moves_media_on_update')) {
            $filesystem->syncMediaPath($media);            <---------- here
        }

This shouldn't be erroring out because a sync method shouldn't complain it can't move a file that doesn't need moving.

The example code that causes this to occur:

        $avatar = $this->addMediaFromBase64($base64Avatar)
                    ->toMediaCollection('avatar');

        $ext = MimeTypes::getDefault()->getExtensions($avatar->mime_type)[0] ?? null;
        $filename = pathinfo($avatar->file_name, PATHINFO_FILENAME);

        if ($ext) {
            $avatar ->file_name = "{$filename}.{$ext}";
            $avatar ->save();                             <--- crash at this point
        }

[freekmurze]

I'd accept a PR that fixes this.

[SlyDave]

Interestingly inside syncMediaPath, there is an attempt to check if ($oldMedia->getPath() === $media->getPath()) but that's not working as intended, I'll see what I can do.

Indeed, if you change the filename, getPath returns a different result

You would expect it to try and move it, via ->move($factory->getPath($oldMedia), $factory->getPath($media)); which fails (against AWS S3).

So this is an S3 issue only, the irony here being that it's recommend to set moves_media_on_update if you're using cloud storage.

I found this issue on flysystem about the move() method of the s3 adapter - basically, because S3 doesn't have the concept of folders, move doesn't work, don't use it.

The issue in question is closed with Closing this issue because AWS doesn't allow this.

[SlyDave]

As a test, I set moves_media_on_update to false which will prevent syncMediaPath() from running but will still allow syncFileNames() to run, and that works?!

The move command produced by syncMediaPath():

signatures/c8877ff3-c37c-42bc-baee-a6e61a0f66f2/4ff2b043646bcb8bd9a000ebb6303cea.tmp -> signatures/c8877ff3-c37c-42bc-baee-a6e61a0f66f2/4ff2b043646bcb8bd9a000ebb6303cea.png
// FAILS

The move command produced by syncFileNames():

909a781c-93cb-4cd2-8723-46fa1d2a749a//d97a28c0617ff1fbe936c59eaf5212ca.tmp -> 909a781c-93cb-4cd2-8723-46fa1d2a749a//d97a28c0617ff1fbe936c59eaf5212ca.png
// SUCCEEDS

(You'll note there is also a bug here, where an extra / is being added to the path because of how the syncFileNames() function rebuilds the path)

The failing one is the one that contains the disk's root, the working one is the one that doesn't. likely because calling move() on the disk puts the root back into it, so the none working one gets the root appended twice? (Speculation at this point)

I also don't know why you'd want both syncMediaPath() and syncFileNames() to both run, it would make more sense that if moves_media_on_update was set to true, it would also do any file_name changes at the same time, doing this would avoid needlessly moving a file twice (once to move its path, once to rename it)

More questions than answers at this point.

[SlyDave]

I think I've found the issue.

The test to see if the file should be moved between folders is done via $oldMedia->getPath() === $media->getPath() but getPath() also includes the filename because it's badly named in the SPL spec.

The actual move is done via $factory->getPath($oldMedia) and $factory->getPath($media); these functions return only the Path not the filename.

As such, the test to see if the move should happen and the move do not use the same paths. hence it is failing.

When if ($oldMedia->getPath() === $media->getPath()) { is replaced with if ($factory->getPath($oldMedia) === $factory->getPath($media)) { everything works as expected.

Incoming PR.

[SlyDave]

PRs for both main and v9 as we need this fixed in the branch that supports PHP7.4.

Worth noting that a bunch of tests in v9 were failing before I made any changes, and the same tests were failing after I made my changes.

[freekmurze]

Thanks!