Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.2] Add Files folder to Media component and to "FileSystem local" adapter #43532

Open
wants to merge 16 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented May 26, 2024

Pull Request for Issue # .

Summary of Changes

Trying to add /files folder, addittionaly to existing /images folder.
It is not very logical to have PDF (and other) documents under /images.

The changes affects new installations. Existing installations should continue to work as usual.

Testing Instructions

Check that Media manager works.
Upload images/files in Media manager view, upload images using Media field in Article editing, etc.

Actual result BEFORE applying this Pull Request

Works

Expected result AFTER applying this Pull Request

Works

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org: TBD
  • No documentation changes for manual.joomla.org needed

@dgrammatiko
Copy link
Contributor

@Fedik please add the files folder also here:

$localDirectories = (new Registry($local->params))->get('directories', [(object)['directory' => 'images']]);

@dgrammatiko
Copy link
Contributor

FWIW I would prefer the folder to be named user_uploads than files

@Fedik
Copy link
Member Author

Fedik commented May 26, 2024

Added.

I think files is a good name, it also will produce a good links.
Let's say Company Site uploads a document, then link will be kind of example.com/files/foo-bar.pdf.
example.com/user_uploads/foo-bar.pdf will not looks "professional" 😉

@Fedik
Copy link
Member Author

Fedik commented May 26, 2024

Currently there still a limitation (pretty huge), the custom field for Media does not allow to select anything else than folders under /images.
It is hardcoded

name="directory"
type="folderlist"
label="PLG_FIELDS_MEDIA_PARAMS_DIRECTORY_LABEL"
directory="images"

I think it will need another field to allow to pick folders from Media manager adapters, but that something for another PR.

@dgrammatiko
Copy link
Contributor

Currently there still a limitation (pretty huge), the custom field for Media does not allow to select anything else than folders under /images

You're right, there's a @TODO for this:

<joomla-field-media class="field-media-wrapper" type="image" <?php // @TODO add this attribute to the field in order to use it for all media types ?>

Co-authored-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>
@dgrammatiko
Copy link
Contributor

One more thing, could you add an .htaccess file inside the files folder with content:

<Files *.php>
  deny from all
</Files>

Since this is a new folder we can restrict it to ONLY static files. (the same should be applied to images and media folders but that's irrelevant to this PR)

@Fedik
Copy link
Member Author

Fedik commented May 26, 2024

there's a TODO for this

Actualy, that "todo" can be already trashed, since we have type="images,documents", attribute.
What i meant, is media field configuration for Custom fields.

One more thing, could you add an .htaccess file inside the files folder

Hmhm, I can, but this can affect people who already have /files folder, and use it on its own purpuse.
Not sure here.

@richard67
Copy link
Member

It seems the system tests need to be adapted, too. Currently they fail at file api/com_media/Files.cy.js:

Test that media files API endpoint
    1) can deliver a list of files
    2) can deliver a list of files in a subfolder
    ✓ can deliver a list of files with an adapter (346ms)
    3) can search in filenames
    4) can deliver a single file
    5) can deliver a single file with the url
    6) can deliver a single folder
    7) can create a file without adapter
    8) can create a folder without adapter
    9) can create a file with adapter
    10) can create a folder with adapter
    11) can update a file without adapter
    12) can update a folder without adapter
    13) can update a file with adapter
    14) can update a folder with adapter
    15) can delete a file without adapter
    16) can delete a folder without adapter
    ✓ can delete a file with adapter (325ms)
    ✓ can delete a folder with adapter (315ms)

  3 passing (38s)
  16 failing

@Fedik
Copy link
Member Author

Fedik commented May 26, 2024

It seems the system tests need to be adapted, too

Yeap, I will look on it later

@brianteeman
Copy link
Contributor

image

@Fedik
Copy link
Member Author

Fedik commented May 26, 2024

@brianteeman ?

@laoneo
Copy link
Member

laoneo commented May 30, 2024

I really like it, just the name is for me too generic. I would rather go with documents, if you name them files, then should the images also be moved there. Or you can go with /files/images and /files/documents. But having a generic files in root which directly contains everything except images is for me not logical.

@Fedik
Copy link
Member Author

Fedik commented May 30, 2024

I will keep it generic.
We definitely can do /files/images, sometime in future. 1 step at time :)

@Fedik
Copy link
Member Author

Fedik commented May 30, 2024

I found a bug in media manager API, it always return path without adapter

And when you doing API call with non default adapter,
like rename folder local-potato:/folder to local-potato:/folder-new it produces 404 error.
Because it looking in default adapter local-files:/folder-new instead of requested local-potato:/folder-new.

@brianteeman
Copy link
Contributor

I am a bit confused about the need for this PR.

If I create a folder myself in the root of the web space then I can already achieve everything that this PR does - what am i missing?

@Fedik
Copy link
Member Author

Fedik commented May 30, 2024

what am i missing?

You need to create the folder by yourself. That what you missing 😉
I offer to have it in the core by default, so you do not have to create it by yourself every time.

@brianteeman
Copy link
Contributor

what am i missing?

You need to create the folder by yourself. That what you missing 😉 I offer to have it in the core by default, so you do not have to create it by yourself every time.

Then I dont see the point in adding this

@Fedik
Copy link
Member Author

Fedik commented May 30, 2024

I do not know, man. I do not know what to say.
It is absolutely basic thing that should be done a decades ago.

Look, we have allowed uploads bmp,gif,jpg,jpeg,png,webp,avif,ico, mp3,m4a,mp4a,ogg, mp4,mp4v,mpeg,mov, odg,odp,ods,odt,pdf, png,ppt,txt,xcf,xls,csv
And by default all goes in to /images folder, it confuses newcomer very much.

There still limitation that need to address, like hardcoded /images path in many places, but that for the future.

@brianteeman
Copy link
Contributor

It is absolutely basic thing that should be done a decades ago.

It was and we removed it

@sousa9g
Copy link

sousa9g commented May 31, 2024

From a Joomla extension developer view, this change is much needed. It is helpful not only for the core com_media. Thank you for adding it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants