[#40691] - [4.3] Media Manager Regression
- Closed
- 2 Jun 2023
- Medium
- Build: 4.3-dev
- # 40691
- Diff
- dgrammatiko:4.3-dev-xcvcvxc
User tests: Successful: Unsuccessful:
Pull Request for Issue #40690 .
Summary of Changes
- The url/session path is not respected
Testing Instructions
- test a url like (existing path)
/administrator/index.php?option=com_media&path=local-images:/banners - test a url like (not existing path)
/administrator/index.php?option=com_media&path=local-none:/banners - test a url like (not existing path)
/administrator/index.php?option=com_media&path=local-none:/noooooo - test a url like (not existing path)
/administrator/index.php?option=com_media - Open the browser console and edit the session value from something like:
{"selectedDirectory":"local-images:/banners","showInfoBar":false,"listView":"grid","gridSize":"md","search":"","sortBy":"name","sortDirection":"asc"}to values matching the changes on the previous steps (eg"selectedDirectory":"local-none:/banners", "selectedDirectory":"local-none:/noooooo") and check if the media manager loads without an error - Apply all possible combos between the url and the session storage
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
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:
-
No documentation changes for manual.joomla.org needed
@laoneo please check all possible combinations between stored session/given url
joomla-cms-bot
- | Category | ⇒ | JavaScript Administration com_media NPM Change |
| Status | New | ⇒ | Pending |
| Labels |
Added:
NPM Resource Changed
PR-4.3-dev
|
||
Thanks for the fast pr. Do we really want to make another api call?
The reason is to never get in a blank screen (ie switching adapters, removing adapters, etc). Also in the request we only get the headers, so it should be fast (maybe it's possible to get the JSON and profile the store, I need to check how the initial fetch is done)
Closing here, there's another PR which patches parts of the buggy behavior
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-06-02 12:36:16 |
| Closed_By | ⇒ | dgrammatiko | |
| Labels |
Added:
bug
|
||
Can this not be done in a way that, when the server returns a 404, it goes to the session path? Having an extra check query for every list files request can lead to a resource issue.
Allon, the problem is that the Store should not start with empty directories/files as it does with either this PR or yours. That is the real problem (because there's no code in the media manager to cope with such state). So, a proper solution (not even this one) is to have the state pre filled with the correct data (in case of a missing folder or mistyped or... you get fallback, as I was trying to do with the previous and this PR). Also in such way you get only 1 fetch for existing folders and more (depending on the fallback code) for missing/mistyped paths.
It does fallback to the first disc or not?
It does fallback to the first disc or not?
It's up to the fallback code, I was lazy and hardcoded it to the first disc but could be more clever (ie if the given disc exists fallback to the root of that drive, etc)
For me that sounds right. I was more thinking about to add more logic here https://github.com/joomla/joomla-cms/blob/4.3-dev/administrator/components/com_media/resources/scripts/app/Api.es6.js#L139. When a 404 is returned, then it should got to another location, either what is in the session or the root folder.
I wouldn't do that in the Api, as it would affect all fetches. The important part is not to start the media manager with invalid state (easy to happen with the current code). For the code you linked above I haven't check the errorHandler but I would expect if there's a 4xx~5xx the selected folder remains the previous one, if it changes to non existing folder then that needs also patching
Thanks for the fast pr. Do we really want to make another api call? I would say it is enough to just override the session parameter with the one from the url, or do I miss here something?