[#34069] - [4.0] refactor the media adapters
- Fixed in Code Base
- 22 May 2021
- Medium
- Build: 4.0-dev
- # 34069
- Diff
- dgrammatiko:—refactor-media-adapters-4.0-dev
User tests: Successful: Unsuccessful:
Pull Request for Issue #26750 (comment).
Summary of Changes
This is a proposal to change the adapters from ie: local-0 to ie: local-images as sated #26750 (comment).
Probably if this is ok (I think the second part would be better if it's md5($name)) would be merged to #33724 for easier testing?
@joomdonation @Fedik @bembelimen your thoughts here?
Testing Instructions
Goto: administrator/index.php?option=com_plugins&view=plugins&filters=[type=filesystem]
Edit FileSystem - Local plugin by adding a new path, eg installation
Check the media manager
Re-edit the previous plugin by changing the order of the folders (first installation, second images)
Check again the media manager still operates fine
Also, edit any article (or create a new one):
- with tinMCE try to upload an image with drag and drop
- try to select an image for intro or full position
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
There shouldn't be any difference for end-users. This PR just ensures that the adapters are not index related.
Documentation Changes Required
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | JavaScript Administration com_media NPM Change Front End Plugins |
| Labels |
Added:
NPM Resource Changed
?
|
||
| Category | JavaScript Administration com_media NPM Change Front End Plugins | ⇒ | JavaScript Administration com_media NPM Change Front End Plugins Unit Tests |
| Labels |
Added:
?
|
||
@dgrammatiko The linter is still a bit unhappy
That was an easy fix but this PR is breaking B/C. (Anyways I was asked if I could help to transition from the index-based naming to this new name-based, so I guess this is already justified)
| Labels |
Added:
?
Removed: ? |
||
looks good, if it works ;)
I not remember where exactly, in past I seen places where second part of the adapter is cast to integer,
I mean stuff like:
$blabla = (int) $part[1];
| Labels |
Added:
?
Removed: ? |
||
I not remember where exactly, in past I seen places where second part of the adapter is cast to integer,
A quick search returned no casting (int) results in com_media or the plugin
A quick search returned no casting
Maybe I mixed something, thanks for checking
| Labels |
Added:
?
Removed: ? |
||
| Labels |
Added:
?
Removed: ? |
||
Thanks @dgrammatiko. All good. I will now run a final test.
Sorry. Could you please do a final change https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/form/field/media.php#L107 ? Change local-0 to local-images
Later it will be changed anyway. But this is to avoid when your PR merged, people use nightly build won't get error while trying to select an image for article.
| Category | JavaScript Administration com_media NPM Change Front End Plugins Unit Tests | ⇒ | JavaScript Administration com_media NPM Change Layout Front End Plugins Unit Tests |
I have tested this item
Thanks, all good now. Great work !
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34069.
| Labels |
Added:
?
Removed: ? |
||
| Category | JavaScript Administration com_media NPM Change Front End Plugins Unit Tests Layout | ⇒ | JavaScript Administration com_media NPM Change Repository Layout Front End Plugins Unit Tests |
@richard67 I guess this one should have a RB tag as it partially solves one of the issues of #26750 The complete solution will be a combo of this and #33724 from @joomdonation. (I guess you can also skip as most probably people are already aware of the state and why this PR created in the first place
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34069.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34069.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-22 09:54:39 |
| Closed_By | ⇒ | Fedik | |
| Labels |
Added:
?
?
?
Removed: ? |
||
Thanks!
hold the front page...
I got this error a few times but its gone now
Thats what I got for ages... - its cache though... its still calling urls with
administrator/index.php?option=com_media&format=json&task=api.files&path=local-0:/
seems Joomla 4 cache busing is not that good at the moment.
There's a sessionStorage entry, so unless you do some update without killing the session then this shouldn't be a problem
actually Im not convinced it is cache... go to Home Dashboard, and click the media icon in the dashboard
Links to http://127.0.0.1:4444/administrator/index.php?option=com_media
it redirects to http://127.0.0.1:4444/administrator/index.php?option=com_media&path=local-0:/
ah - let me try killing that
yup that was it - clear as day it had local-0 in the sessionStorage entry,
So if someone had a Joomla 4 beta, and had been to media in this session, and upgrades to Joomla 4 RC1 in the same session, they COULD have this problem right?
Yeah, but I think it's highly unlikely to hit all these requirements at once...
Yeah, but I think it's highly unlikely to hit all these requirements at once...
/bookmarked....
Highly unlikely as in two people already did
Is there anything we can do against it?
Is there anything we can do against it?
We could empty the sessionStorage key before the update, but it won't cover all the updade paths
But we can ask people to log out after the update and re-login (in case they used com_media right before the update). No code needed
@dgrammatiko The linter is still a bit unhappy: https://ci.joomla.org/joomla/joomla-cms/44184/1/21 :