search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[UX] Provide a report page that lists all configured paths where files are being saved. #4585

Open klonos opened 4 years ago

klonos commented 4 years ago

@olafgrabienski in https://github.com/backdrop/backdrop-issues/issues/4583#issuecomment-683414706

... In text format configuration (e.g. admin/config/content/formats/filtered_html) you can "Enable file uploads", set an Upload directory and more. It's quite hidden, and I often forget this option when building new sites.

We should have a report page, where site builders/admins can get an overview of all the various directories where files are being saved for text formats, media types, file/image fields etc.

Advocate: @klonos

klonos commented 3 years ago

@stpaultim I thought we should get this one out of the way as well: https://github.com/backdrop/backdrop/pull/3634

Here's what I have so far: image

...and with some more directories + private dir configured: image

klonos commented 3 years ago

...feedback I'm looking for:

Things I need to work on:

indigoxela commented 3 years ago

I did some super sloppy testing in the sandbox. The first thing that caught my eye:

Notice: Undefined index: file_upload in file_upload_dir_list() (line 699 of /var/lib/tugboat/core/modules/file/file.admin.inc).
klonos commented 3 years ago

@indigoxela thanks for testing this. I can do some defensive coding, but curious to know how you managed to trigger that error.

indigoxela commented 3 years ago

curious to know how you managed to trigger that error

Nothing special, I turned on error display and went to https://pr3634-05xu7lvykqhrckpa6sgi8q3sg7agcb9r.tugboat.qa/admin/reports/file_upload_dir_list

klonos commented 3 years ago

Huh 👀

klonos commented 3 years ago

Thanks @indigoxela 🙏🏼 ...fixed.

stpaultim commented 3 years ago

I'm just looking at this in the sandbox. Looks helpful.

I have not had time to process what I'm seeing yet, as I'm a bit overwhelmed with other things right now, but have some quick feedback.

Would "Access" work better than "Storage"? It might not be technically correct, but it feels like public/private is an access issue, although different than other access issues in Backdrop. I think that "Storage" is fine, since anyone reading this page will (I think) understand the difference between public/private regardless of the label.

I notice that the "temp" directory is not listed. Is there a reason for that?

klonos commented 3 years ago

I have not had time to process what I'm seeing yet, as I'm a bit overwhelmed with other things right now...

Right ...I meant to mention @olafgrabienski, since he was the one that requested this; sorry I mentioned you instead @stpaultim, but thanks for taking the time to test anyway 😅

In theory, there may be more "stream wrappers" than just public/private, so I'm not sure "Storage" would cover all these cases.

I notice that the "temp" directory is not listed. Is there a reason for that?

The temp directory is only used as (as the name suggests) a temporary storage until the file is uploaded/saved. This folder doesn't actually hold any media assets in the CMS. I could include it in the list, but then we'd need to come up with a better name for the report, which I'm currently calling "File upload directories" 🤷🏼

indigoxela commented 3 years ago

Would "Access" work better than "Storage"?

IMO "storage" is correct. (Not a native speaker, though.)

I took a look at the file system settings page (help text):

 A local file system path where public files will be stored.
 An existing local file system path for storing private files.

So, "storage" seems consistent to me - we're using the same term there.

In theory, there may be more "stream wrappers" than just public/private

But if files get stored there (wherever it is), it's also a "storage", right?

olafgrabienski commented 3 years ago

I meant to mention @olafgrabienski, since he was the one that requested this

@klonos Thanks for working on this issue! I'm also busy with other things and can't test right now. I wanted however mention that I didn't "request" anything in regard of this issue, see the discussion starting at https://github.com/backdrop/backdrop-issues/issues/4583#issuecomment-683414706.

klonos commented 3 years ago

But if files get stored there (wherever it is), it's also a "storage", right?

Yup, that's where I was getting at 👍🏼

... I didn't "request" anything in regard of this issue...

Oops! Sorry if I interpreted that this way. Still a nice feature though, right? 😅

I'll wait for people to have more time to test this and get more feedback/ideas/thoughts then. Thanks everyone for having a look 🙏🏼

klonos commented 3 years ago

Note to self: from the dev meeting today, @quicksketch suggested making sure that contrib can use this feature, to add any locations in the filesystem that are configured to store files uploaded. It could be something similar to https://github.com/indigoxela/backdrop/blob/1.x/core/modules/image/image.api.php#L160

ghost commented 3 years ago

I like the idea. Some thoughts:

klonos commented 3 years ago

But I think we should capitalise the terms here.

Easy 👍🏼 ...do others agree with this though? (so that I don't go back and forth in making changes to the PR). Note that I could also change this to the label of each wrapper, in which case we'd have "Public files" and "Private files". This achieves capitalization, but adds "files" to the string, which makes it more verbose.

Can we simplify that?

Sure. Suggestions? ...please keep in mind that my main goal with that text was to provide inks to the places where each respective path can be edited.

ghost commented 3 years ago

I don't think we need to add 'files' to public/private. That just adds more similar text that makes it harder to distinguish.

As for re-wording, how about just:

klonos commented 3 years ago

Thanks @BWPanda 👍🏼 ...please note that I also output the field type. So unless you have a better idea, I was thinking this:

klonos commented 3 years ago

...I've updated the PR. Here's what we have now:

image

Better?

klonos commented 3 years ago

...last tweak (not in the PR yet): does everyone think that the indicators for default dirs is fine as a bullet point, as in the screenshot in the comment above, or like so:

image

...or does anyone have any better idea re how to indicate those in the list?

ghost commented 3 years ago

Ooh, I like that last one better!

klonos commented 3 years ago

I think I like it better too. I'll wait for some more feedback, and will then update the PR accordingly.

olafgrabienski commented 3 years ago

I also like the version without bullet point better. In this case, I'd prefer a normal font-weight.

stpaultim commented 2 years ago

I also like the version without bullet point better. In this case, I'd prefer a normal font-weight.

I agree.

This seems like a nice report, but I'm not sure that I'd ever use it. I'm not against having it in core, but I'm also not completely convinced of the need for it. Will defer to the opinions of others.

schoenid commented 2 years ago

Well I would like to have it. Does it support paths modified by https://github.com/backdrop-contrib/filefield_paths? This is using tokens, to be able to spread the paths, which is required, if you else have thousands of files in the same location.

ghost commented 2 years ago

There's a conflict in the PR, and while I could fix it myself in @klonos' absence, it seems that the PR is changing files outside of the scope of this issue... So I think we need to wait for @klonos to come and clear this up. Marking as NW until then.