Skip to content

[5.2] MediaManager: improve error handling#38536

Merged
Hackwar merged 10 commits intojoomla:5.2-devfrom
Fedik:mediamanager-erroring
Jan 18, 2025
Merged

[5.2] MediaManager: improve error handling#38536
Hackwar merged 10 commits intojoomla:5.2-devfrom
Fedik:mediamanager-erroring

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Aug 19, 2022

Pull Request for Issue #38068 and #38530 .

Summary of Changes

Improve error handling in MediaManager

Testing Instructions

Apply patch. Run npm install.
Try upload something not allowed in to mediamanager.

Actual result BEFORE applying this Pull Request

General error Unable to upload file.

Expected result AFTER applying this Pull Request

General error Unable to upload file.
Then multiple detailed errors:
Or Illegal mime type detected or This file type is not supported (depend from medimanager config)
Or for svg: The file looks suspicious, therefore cannot be uploaded

Documentation Changes Required

nope

@brianteeman please review an updated string.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Aug 19, 2022
@laoneo
Copy link
Member

laoneo commented Aug 19, 2022

Nice one as it will catch on every request the error messages.

@obuisard
Copy link
Contributor

Thank you for providing a better solution!

@brianteeman
Copy link
Contributor

even better would be to display the error message from the svgsanitizer which explains exactly why the file was rejected

@obuisard
Copy link
Contributor

I have added documentation about uploading SVG files. You can find it at https://docs.joomla.org/J4.x:Media:_Uploading_SVG_files. It may need refining and additional content. You are welcome to pitch in, thank you!

@Hackwar Hackwar added the Small A PR which only has a small change label Feb 26, 2023
@brianteeman
Copy link
Contributor

IT does what it says but I wonder why we have the two messages. The second one makes the first one redundant?

image

@Fedik
Copy link
Member Author

Fedik commented Mar 14, 2023

The script render all messages that was sent from PHP backend.
In your example, I guess the first one comes from a controller, and second one from a validator.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on b5feda2


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

@Fedik
Copy link
Member Author

Fedik commented Mar 14, 2023

Before the fix, the script was rendered only first message, and rest were ignored.

@joomdonation
Copy link
Contributor

In your example, I guess the first one comes from a controller, and second one from a validator

The first message is from the Exception https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/filesystem/local/src/Adapter/LocalAdapter.php#L772 , additional messages are Joomla system messages generated by calling $app->enqueueMessage from canUpload from MediaHelper https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Helper/MediaHelper.php#L200

Still the first error message is redundant as Brian said. Is it OK to make the Exception https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/filesystem/local/src/Adapter/LocalAdapter.php#L772 with empty message, and have the Joomla system messages rendered for this case ? It is kind of workaround but more user friendly.

@Fedik
Copy link
Member Author

Fedik commented Mar 20, 2023

Is it OK to make the Exception with empty message

I do not think.
I think the root issue that $helper->canUpload() should return boolean and cannot throw an exception, so someone decided to put all in to $app->enqueueMessage . Not perfect, but working.

@joomdonation
Copy link
Contributor

I have tested this item ✅ successfully on b5feda2

Not perfect but work.


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

@joomla-cms-bot joomla-cms-bot removed the Small A PR which only has a small change label Mar 24, 2023
@joomdonation
Copy link
Contributor

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 24, 2023
@Hackwar Hackwar added the bug label Apr 6, 2023
@brianteeman
Copy link
Contributor

@AmyKrizanWang please do not update your comment. It is nothing to do with this issue.

@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:29
@HLeithner
Copy link
Member

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

Fedik added 3 commits May 3, 2023 13:51
@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Aug 25, 2023
@HLeithner HLeithner changed the base branch from 4.3-dev to 4.4-dev September 30, 2023 22:45
@HLeithner
Copy link
Member

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

@HLeithner HLeithner changed the title MediaManager: improve error handling [4.4] MediaManager: improve error handling Apr 24, 2024
@AmyKrizanWang
Copy link

@AmyKrizanWang please do not update your comment. It is nothing to do with this issue.

comment removed

@HLeithner HLeithner changed the base branch from 4.4-dev to 5.2-dev November 15, 2024 13:22
@HLeithner
Copy link
Member

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

@HLeithner HLeithner changed the title [4.4] MediaManager: improve error handling [5.2] MediaManager: improve error handling Nov 15, 2024
@richard67 richard67 removed PBF Pizza, Bugs and Fun PR-4.4-dev labels Jan 13, 2025
@pe7er
Copy link
Contributor

pe7er commented Jan 18, 2025

I have tested this item ✅ successfully on 2e13c16

Before PR

before-pr

After PR (and recompiling with npm ci)

after-pr


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

@MacJoom
Copy link
Contributor

MacJoom commented Jan 18, 2025

I have tested this item ✅ successfully on 8b01df6

Upload of a suspicous svg failed with the new messages. Ordinary svg worked


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 18, 2025
@Hackwar Hackwar enabled auto-merge (squash) January 18, 2025 20:29
@Hackwar Hackwar merged commit 664f199 into joomla:5.2-dev Jan 18, 2025
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 18, 2025
@Hackwar Hackwar added this to the Joomla! 5.2.4 milestone Jan 18, 2025
@Hackwar
Copy link
Member

Hackwar commented Jan 18, 2025

Thank you!

@Fedik Fedik deleted the mediamanager-erroring branch January 19, 2025 08:45
Kostelano added a commit to JPathRu/localisation that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester Small A PR which only has a small change

Projects

None yet

Development

Successfully merging this pull request may close these issues.