Skip to content

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Feb 10, 2025

Summary of Changes

The PR changes the media manager to use FormData for file upload, instead of base64 payload.
Which improves responsiveness, performance and handling of large files in general.

However the api/ calls still uses base64 because it is hard to do in b/c maner.

Bonus: the upload progres now showing actual upload progress.

Testing Instructions

Apply patch.
Run npm install.
Go to media manager and try upload something bigger than 100Mb

Addittionaly, test with any 3d filesystem plugin.

Actual result BEFORE applying this Pull Request

The browser hangs for a while, and dependent from your luck the upload will be completed after some waiting.

Expected result AFTER applying this Pull Request

The upload starts immediately, and you can see progress (a litle 5px blue line over the files list)

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: Media: large file upload Manual#431
  • No documentation changes for manual.joomla.org needed

@Fedik Fedik changed the title [5.3] Media: fix large file upload with use of binary upload instead of base64, and display correct upload progress [5.3] Media: fix large file upload by using of binary upload instead of base64, and display correct upload progress Feb 10, 2025
@laoneo
Copy link
Member

laoneo commented Feb 11, 2025

I like that change, but this will break 3rd party extensions when the data can be all of the sudden a resource. I would read the file content it in the api controller and then pass it to the adapter the same way as before that no changes are needed in the adapter. Additionally, the check content logic can probably be moved to the, but not sure about this.

@richard67
Copy link
Member

Additionally, the check content logic can probably be moved to the, but not sure about this.

@laoneo Moved to the ... what?

@laoneo
Copy link
Member

laoneo commented Feb 11, 2025

ApiController

@Fedik
Copy link
Member Author

Fedik commented Feb 11, 2025

Hmhm,

I would read the file content it in the api controller and then pass it to the adapter the same way as before that no changes are needed in the adapter

No no, this not going to work, you will get "out of memory" error when try to read 100Mb file when PHP limit is 60Mb.
This will bring back problem with large file upload.
Loading whole file in to memory should never happen while uploading (unless when it realy need).

What about stringable object?
We wrap uploaded file in to that object, and the file will be loaded in to memory only when the plugin expecting a string. Addittionaly we deprecate this behavior.
What do you think?

@laoneo
Copy link
Member

laoneo commented Feb 11, 2025

Not sure if stringable would solve it. But I think the main goal should be to do it in a bc way.

@Fedik
Copy link
Member Author

Fedik commented Feb 11, 2025

I updated the code to use stringable object.
In my tests it works well even when try to access the data as to the string.

Would be good to have some more test, to be sure, with 3rd filesystem adapters.

@Tejashrimajage
Copy link

I have not tested this item.

Pathch not applied. Got error The file marked for modification does not exist: administrator/components/com_media/resources/scripts/app/Api.es6.js while applying patch


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

@Tejashrimajage
Copy link

I have not tested this item.

Got error while applying patch.The file marked for modification does not exist: administrator/components/com_media/resources/scripts/app/Api.es6.js


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

 Conflicts:
	plugins/filesystem/local/src/Adapter/LocalAdapter.php
@HLeithner HLeithner changed the base branch from 5.3-dev to 6.0-dev March 4, 2025 17:18
@HLeithner HLeithner requested a review from rdeutz as a code owner March 4, 2025 17:18
@HLeithner
Copy link
Member

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

@HLeithner HLeithner changed the title [5.3] Media: fix large file upload by using of binary upload instead of base64, and display correct upload progress [6.0] Media: fix large file upload by using of binary upload instead of base64, and display correct upload progress Mar 4, 2025
@rdeutz rdeutz removed the PR-5.3-dev label Mar 5, 2025
@HLeithner HLeithner changed the base branch from 6.0-dev to 6.1-dev August 31, 2025 11:57
@HLeithner
Copy link
Member

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

@HLeithner HLeithner changed the title [6.0] Media: fix large file upload by using of binary upload instead of base64, and display correct upload progress [6.1] Media: fix large file upload by using of binary upload instead of base64, and display correct upload progress Aug 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

10 participants