Skip to content

Conversation

@mattelkins-bluefrontier
Copy link
Contributor

@mattelkins-bluefrontier mattelkins-bluefrontier commented Aug 14, 2024

Summary of Changes

  • If mediatypes is not specified in the input, fallback to an explodable string of all supported formats (0,1,2,3) rather than images only (0).

Testing Instructions

  1. Download the following file: Example MP4
  2. Upload the video file in to the root of the media library - by default, the path to the file should be: local-images:/file_example_MP4_480_1_5MG.mp4
  3. In a custom component or module (e.g. mod_test_43915.zip created by @degobbis), call:
    \Joomla\Component\Media\Administrator\Model\FileModel->getFileInformation('local-images:/file_example_MP4_480_1_5MG.mp4');

Actual result BEFORE applying this Pull Request

With mediatypes not present or empty in the input object, calling FileModel::getFileInformation() with a path to anything other than an image file will generate an InvalidPathException.

The exception is thrown by ApiModel::getFile(), and will also be thrown if this function is called directly with a separate adapter and path.

The exception is triggered due to the fact that ApiModel::isMediaFile() will return false for a non-image file if mediatypes is not present or is empty in the input object. Without mediatypes, allowed formats will be limited to images only, rather than images, audio files, videos and documents.

Expected result AFTER applying this Pull Request

FileModel::getFileInformation() (and ApiModel::getFile()) will return an object containing file information for image and non-image files, assuming the referenced file exists at the location specified by the path.

(The namespace for all classes mentioned above is: Joomla\Component\Media\Administrator\Model.)

Link to documentations

No documentation changes for docs.joomla.org needed.
No documentation changes for manual.joomla.org needed.

@mattelkins-bluefrontier
Copy link
Contributor Author

Reopened against 5.2-dev after accidentally closing #43640 because I'm an idiot. 🤦‍♂️

This PR incorporates feedback from @bembelimen in this comment.

In the scenario where mediatypes is an empty string, ...->getInput()->getString() will return an empty string rather than the specified default value, the consequence of which is that explode() will return an array containing one empty value, i.e.:

array (
    0 => ''
)

array_filter() is used to remove all empty values from the array. The following \count($mediaTypes) === 0 will then be true, and $mediaTypes will be set to an array containing all supported file formats.

@ghost
Copy link

ghost commented Aug 16, 2024

  1. Upload the video file in to the root of the media library - by default, the path to the file should be: local-images:/file_example_MP4_480_1_5MG.mp4

Using PR or not got: "Unable to upload file."

Test System Information:

Sample Data – Testing
PHP Built On Darwin Air.local 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:13:00 PDT 2024; root:xnu-10063.141.2~1/RELEASE_X86_64 x86_64
Database Type mysql
Database Version 8.0.35
Database Collation utf8mb4_unicode_ci
Database Connection Collation utf8mb4_0900_ai_ci
Database Connection Encryption None
Database Server Supports Connection Encryption Yes
PHP Version 8.3.8
Web Server Apache/2.4.58 (Unix) OpenSSL/1.1.1u mod_fastcgi/mod_fastcgi-SNAP-0910052141
WebServer to PHP Interface cgi-fcgi
Joomla! Version Joomla! 5.2.0-alpha4-dev Development [ Uthabiti ] 23-July-2024 16:01 GMT
Joomla Backward Compatibility Plugin Disabled
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:129.0) Gecko/20100101 Firefox/129.0

@crommie
Copy link

crommie commented Aug 24, 2024

@mattelkins-bluefrontier could you elaborate on #3 of the testing instruction ("In a custom component or module, call \Joomla\Component\Media\Administrator\Model\FileModel::getFileInformation('local-images:/file_example_MP4_480_1_5MG.mp4');")? How do you do that?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43915.

@exlemor
Copy link

exlemor commented Aug 24, 2024

This PR requires coding a custom module or component and not a mere easy PR test.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43915.

@degobbis
Copy link
Contributor

Please correct Your testing instructions, calling the methode \Joomla\Component\Media\Administrator\Model\FileModel::getFileInformation('local-images:/file_example_MP4_480_1_5MG.mp4'); statically is not more possible.
I also have created a module to testing this:
mod_test_43915.zip

@degobbis
Copy link
Contributor

I have tested this item ✅ successfully on 72f667f

Calling the method statically is not more possible, so using my test module will reproduce the error.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43915.

@brianteeman
Copy link
Contributor

Calling the method statically is not more possible, so using my test module will reproduce the error.

That means this is a breaking change then doesnt it?

@degobbis
Copy link
Contributor

degobbis commented Aug 24, 2024

That means this is a breaking change then doesnt it?

I revoke my statement that it cannot be called up statically, it does work.
I tested the call when I had not yet uploaded the file and therefore got an error that made me think it was because of that.

I just tested it again successfully with the static call.
Sorry for the confusion.

@brianteeman
Copy link
Contributor

@degobbis thanks for clarifying that

@mattelkins-bluefrontier
Copy link
Contributor Author

Many thanks for all of your comments, and especially to @degobbis for the example module. I've updated the PR description accordingly.

My apologies for the confusion around getFileInformation() - it is not a static method in the FileModel class. This was my mistake - I used the PHPDoc FQSEN in the line of code I provided.

@HLeithner HLeithner changed the base branch from 5.2-dev to 5.3-dev September 2, 2024 08:50
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.3-dev.

@HLeithner HLeithner changed the title [5.2] Ensure isMediaFile defaults to all supported formats when mediatypes is not specified [5.3] Ensure isMediaFile defaults to all supported formats when mediatypes is not specified Sep 2, 2024
@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
@HLeithner HLeithner changed the base branch from 5.3-dev to 6.0-dev March 4, 2025 17:20
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 6.0-dev.

@HLeithner HLeithner changed the title [5.3] Ensure isMediaFile defaults to all supported formats when mediatypes is not specified [6.0] Ensure isMediaFile defaults to all supported formats when mediatypes is not specified Mar 4, 2025
@rdeutz rdeutz removed the PR-5.3-dev label Mar 5, 2025
@exlemor
Copy link

exlemor commented Mar 8, 2025

I have tested this item 🔴 unsuccessfully on 5946845

I tested this unsuccessfully on 6.0.0-alpha1-dev.

Before applying PR, installed the test module, turned it on on all pages in position main-top. Reloaded front page = crash with:

An error has occurred while processing your request.
You may not be able to visit this page because of:

an out-of-date bookmark/favourite
a mistyped address
a search engine that has an out-of-date listing for this site
you have no access to this page
Go to the Home Page

If difficulties persist, please contact the website administrator and report the error below.

0

After applying PR, same exact results :/ ;(


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43915.

@exlemor exlemor moved this to Abandoned in PR Cleanup May 10, 2025
@HLeithner HLeithner changed the base branch from 6.0-dev to 6.1-dev August 31, 2025 11:58
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 6.1-dev.

@HLeithner HLeithner changed the title [6.0] Ensure isMediaFile defaults to all supported formats when mediatypes is not specified [6.1] Ensure isMediaFile defaults to all supported formats when mediatypes is not specified Aug 31, 2025
// Get options from the input or fallback to images only
$mediaTypes = explode(',', Factory::getApplication()->getInput()->getString('mediatypes', '0'));
// Get options from the input or fallback to all supported formats
$mediaTypes = array_filter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback was hardcoded to 0 (images) for backward compatibility. Changing this needs to be done in a major version and after checking that it won’t break the majority of 3rd party extensions. Also it needs a good communication… fwiw I don’t know what exactly it would solve to allow everything by default, good systems are usually restricted by default…

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

Projects

Status: Abandoned

Development

Successfully merging this pull request may close these issues.

10 participants