Posted by sanderjp on May 2, 2011 at 9:16am
10 followers
| Project: | Media |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
This is not a big one, but I couldn't figure it out myself.
I have a Multimedia asset field in a content type and you can upload unlimited files. After you select/upload one file and click the button "Add another item", then click the "select media" again, you get an iframe with admin toolbar which looks abit funny. This did not happen in beta3.
I've found out the source url (widget.src/media.browserUrl) somehow douplicates when you add another item: from "/media/browser?render=media-popup..." to "/media/browser?render=media-popup,/media/browser?render=media-popup..." and 3 times for 3 files etc.
Comments
#1
Well I dived in it myself and found that each time you click "Add another item" it adds more entry's in the element array (in media.browser.inc at function 'media_attach_browser_js()' line 301). The variable browserUrl should be the same for each item so doesn't need to be added for each item seperatly.
The problem is in the function 'Drupal.media.popups.mediaBrowser.getDefaults' in media.popups.js (line 107) where it loads the browserUrl. If there is one item, the variable contains '/media/browser?render=media-popup' but more items turns it into an array and the variable outputs '/media/browser?render=media-popup,/media/browser?render=media-popup'.
My solution is not the best way but as far as I've tested it, it works. In media.popups.js, at the function 'Drupal.media.popups.mediaBrowser.getDefaults' (line 107) i've added an if statement to check wether it's an array or not. If so, only take the first entry of the array:
var browserUrl = Drupal.settings.media.browserUrl;if (browserUrl.constructor.toString().indexOf("Array") != -1) {
browserUrl = Drupal.settings.media.browserUrl[0];
} else {
browserUrl = Drupal.settings.media.browserUrl;
}
And at line 121, I changed:
src: Drupal.settings.media.browserUrl; // Src of the media browser (if you want to totally override it)
into:
src: browserUrl, // Src of the media browser (if you want to totally override it)
As I said for now it works, but I'm not sure this is the best way to solve it.
Feedback/comments/improvements are welcome :)
#2
I would think that the correct fix would be to solve the bug that is turning the url into an array in the first place. Your suggestion fixes the symptom but not the issue.
This will also be irrelevant once #1139514: Overhaul the media browser code to not use an iframe, and be more understandable, maintainable, and extendable makes its way into the main branch.
#3
yea, this is just a workaround, will keep an eye on the fat-trimming :)
#4
Adding sanderjp's work-around as a patch seeing as there's not much point in finding a better solution in parallel with the fat-trimming work that's happening, but there's no telling when that one will make it in and I need this in my make file right now ;-)
#5
re-roll of #4 against HEAD
#6
#5 is not against HEAD. I'm not sure what I was thinking
#7
thx for the work a round in #5.
#8
This is still an error in RC1 - I really needs to be addressed for a final release.
#9
#5 is working fine for me too.
Edit: #4 is working fine for me too.
#10
The last submitted patch, popup-browserurl-1144422-5.patch, failed testing.
#11
#4: 1144422-popup-browserurl.patch queued for re-testing.
#12
I'd rather fix the bug here as well rather than the symptom.