Changing the name of an asset in the control panel displays a Call to a member function path() on null error

JayGeorge commented 3 years ago

Bug Description

I think there are UX problems when renaming files in the control panel but this issue is about the UX around the way Statamic works with image name references—specifically the display of Call to a member function path() on null error when changing the name of an asset in the control panel.

This is somewhat related to these issues:

The forum thread Renaming and Moving Asset Files Should Update References
and the issue #167 Replace asset filenames in markdown after renaming

How to Reproduce

(here is a screencast)

Inside a page or collection that already has an 'image' field type click to edit the image
Click 'Rename File' and change the name to anything e.g. 'test'
Go to save the page
Statamic will throw a Call to a member function path() on null error

Extra Detail

(here is a screencast)

File renaming is presented as a common task in the CP, there should be a big warning set against this if breaks things
Renaming a file and then not being able to save the page or collection feels broken—especially when Statamic throws a technical error that a client wouldn't understand

As I understand it this might be the way that Statamic is designed—the collection or page in the CP is referencing the name of the file rather than an underlying id.

Is there a reason that image names don't refer to each other by ID, the same as page's have a separate 'Name' and 'slug'? For example if I rename a page in the admin I don't have to worry about links breaking or re-architecting my site as a consequence of renaming.

Environment

Statamic 3.0.7 Solo Laravel 7.20.0 PHP 7.4.9 No addons installed

Install method (choose one):

Fresh install from statamic/statamic

jasonvarga commented 3 years ago

Thank you for the detailed issue.

We've weighed the pros and cons of using ids vs. just the paths. Using paths by far outweighed the benefits of IDs.

People love using Statamic by hand in the filesystem. You drop an image in a folder, and put the path in your content. Done.

If we had to track asset IDs, you'd need to plop the file somewhere, update some sort of map of ids to paths, then put the ID in your content. If you never rename assets, then that was a lot of effort for no gain.

Adding a permission to prevent renaming entries is a great workaround for this. This would just prevent the problem in the first place.

It's probably a decent idea to think of assets as mutable. Instead of renaming, maybe just upload a new asset instead.

The cryptic error you see if you rename an asset, I'd definitely consider a bug. We should just filter out assets that don't exist.

JayGeorge commented 3 years ago

Likewise, thanks for the detailed reply! 😄

I can now see it's possible to change asset names by additionally performing a search/replace across the project. That begs the question, why Statamic can't perform a search/replace when renaming in the CP? That would solve this whole issue. I guess this has something to do with performance.

UX

While I can sort of accept the technical decisions behind file links breaking I think there needs to be some additional UX work around renaming assets in the CP for V3 because at the moment the consequences are not clear.

I read somewhere that there used to be a warning about links breaking when renaming assets in the control panel, I wonder what happened to that in v3?

jasonvarga commented 3 years ago

While it's fairly simple to do a find/replace in files, it's not a guarantee that everyone is using files for their content. Things need to be written in an agnostic way, which complicates it.

I've brought back the warning in a1f249ef. It was just something left off unintentionally when we re-implemented the rename modal.

Also to be clear, you can do the permission thing already. It wasn't just a hypothetical thing. You can create a role for your clients and don't give them permission to rename assets.

robdekort commented 3 years ago

Closing this for now since there's a warning added and there's a clear reasoning for the choices. Feel free to reopen @JayGeorge if you think it should be.

statamic / cms