Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I've created two views for the media browser: 1. Image Browser and 2. File Browser (just cloned the default Media browser view and made necessary changes)
If I activate only Image Browser (or any other single view) for a certain field everything works fine. But when I try to activate two or more views for the same field I'm unable to select any file.
Chrome (19.0.1084.41 beta-m) gives me the following error: "Uncaught TypeError: Cannot read property 'fid' of undefined" (media.js:45).
Comment | File | Size | Author |
---|---|---|---|
#29 | 1562164-d7-3.patch | 2.41 KB | redndahead |
#28 | 1562164-d7-2.patch | 2.41 KB | redndahead |
#18 | 1562164-d7-1.patch | 2.41 KB | redndahead |
#16 | media-add-media-view-submit-button-fix-1417436-33.patch | 1.31 KB | damontgomery |
#13 | 1562164-fix-views-file-selection.patch | 2 KB | Dave Reid |
Comments
Comment #1
Equ CreditAttribution: Equ commentedI think I've found what causes the problem.
drupal_add_js is being called for every active media browser view. As a result $javascript['settings']['data'] (static variable from drupal_add_js) holds multiple media files arrays:
All the above causes Drupal.settings.media.files to store files as an array (with indexes starting at 0) instead of an object with properties (file objects).
And finally this:
gives us undefined.
Comment #2
Equ CreditAttribution: Equ commentedHere is a possible solution:
Comment #3
ahmedwali CreditAttribution: ahmedwali commentedI have error when i click file in view library tab
mediaFile is undefined
http://localhost/sites/all/modules/media/js/media.js?m4s0k8 Line 45
Comment #4
Dave ReidYep, I'm encountering this too as well. Putting on a fast-track to fix today.
Comment #5
Dave ReidSo I'm thinking we have two possible solutions here:
1. Apply the temporary fix similar to #2.
2. Change the Media browser to request the JSON file object via AJAX when requested, rather than pre-load every single possible file in Drupal.settings.media.files.
Comment #6
Dave ReidOr, another alternative:
3. Place the JSON inside a HTML5 data attribute in the .media-item row itself. This would be automatically fetchable with http://api.jquery.com/data/#data-html5
Comment #7
fangel CreditAttribution: fangel commentedWhy insist on making it super complicated? Why insist on calling the solution proposed in #2 'a temporary fix'?
Since introducing the 'My files' tab, this bug will always be triggered because now two views-backed displays are shipped by default. Hence the re-key'ing will always happen, so I don't see why you shouldn't just accept that and use it as an array. No need trying to re-cast it to an hash-map style object.
Why not just ensure that
Drupal.settings.media.files
is always an array and then just iterate through the files-array to find the object with it'sfid
-property equalling thedata-fid
-property of the selected image.That is easily doable - simply add an
array_values
in thedrupal_add_js
line inmedia.views.js:88
and by using a simpleArray.filter
to reduce the Javascript-array to only contain the right file.It solve the issue by changing two lines!
I fail to see what added benefits solution #2 or #3 bring? They only bring a ton more complexity - especially solution #2, ajaxifying the data-gathering. So why would you ever go that way, and not just use this very permanent "temporary" fix.
Comment #8
Dave ReidWell holy hell, I'm sorry for ever trying to think an issue through with the possible solutions to ensure that we not only fix the issue, but we fix any underlying issues or architecture behind it. I was going to come back today and update that the "temporary" fix is most definitely the right way to go for now, but I guess you've already decided.
My thinking is that I'm wondering if adding the JSON for each file in Drupal.settings is actually not a great thing to be doing, and using data attributes for the JSON would be better. And for reference, I was able to implement #2 in about 2 minutes and it just works as well.
I'll get on this today with implementing #1.
Comment #9
DamienMcKenna@DaveReid: Take a deep breath, buddy, and go pet Rodney for a while.
@fangel: Thank you for finding a solution to the problem, we all appreciate your effort. While it is a solution, Dave wanted to delve deeper into the problem to find out what was really causing the problem and identify the best solution, and it seems that he's close to working it. out.
Comment #10
fangel CreditAttribution: fangel commentedI wasn't trying to be hostile. I was just confused why the - imho - simple solution was first categorized as temporary.
As far as I know, the current solution of using Drupal.settings has worked great, then problems with
drupal_add_js
re-key'ing the array popped up. So it just seemed strange to want to go out and do a fairly big restructuring to something that - again to my knowledge - has worked splendidly so far, and only requires very few lines to fix.But if you think there are architectural reasons for why something other that the current approach is needed, I believe it can only benefit from these reasons being discussed.
Sorry for my somewhat harsh tone, I just personally see no reason to replace something only slightly broken with something much more complicated and untested without good, known reasons.
Comment #11
Dave ReidNote that the JavaScript filter() function is not available in all browser, specifically IE versions lower than 9: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Arr...
Comment #12
fangel CreditAttribution: fangel commentedFor what it's worth, here's an updated patch that uses a for()-loop instead of filter.
Comment #13
Dave ReidI'm not sure why the array_values() change is necessary. Revised patch with changes only to media.views.js.
Comment #14
Dave ReidMerged #1417436: Submit and Cancel Buttons on Views Tab's don't do anything into this issue which has lots of helpful comments.
Comment #15
fangel CreditAttribution: fangel commentedI had the
array_values()
there to ensure Drupal.settings.media.files was an array, not an object (hashmap-style).If you don't have that, and only have one call to
drupal_add_js
, then Drupal.settings.media.files will have fid-as-keys, which results in it being encoded as a Javascript object (hashmap), and then the for-loop wont work, because it's trying to traverse over an object, and not an array.Comment #16
damontgomery CreditAttribution: damontgomery commentedI'm copying this from the duplicate because this is where we ended up,
http://drupal.org/node/1417436#comment-6055488
I think this is still relevant because the previous patches still look like they are using the "parent" traversal which breaks in the situations explained above.
Comment #17
damontgomery CreditAttribution: damontgomery commentedI think combining #15 and #16 makes sense to me to ensure the patch works for single and double views. Fangle's comments make sense. It was also proposed in the other thread to use a string replace function instead of substr(), which I think makes sense to make it more readable, although both will fail if the ID naming changes.
I feel that these fixes should be rolled out since the views are currently broken, but that the underlying structure needs to be eventually fixed. The broken A inside A is one of the issues here and even with the fix in #16, you can't click on the thumbnail to select the media.
Comment #18
redndahead CreditAttribution: redndahead commentedThis one combines #13, suggestions from #15, #16 and I think I improved the fid finding code. Give it a shot.
Comment #19
damontgomery CreditAttribution: damontgomery commentedredndahead,
This traversal will not work for my install because some of the media files have no wrapper anchor.
Is anyone else having this issue? Have you tried with PDF / Image / Video media files?
Comment #20
Dave Reid@pandaeskimo: Hrm, I can't confirm not having a wraper anchor. Did you change the 'Preview' view mode display for any file types? Or are you using the File styles module at all?
Comment #21
redndahead CreditAttribution: redndahead commentedWe have separate views for images, videos, files and docs. It's almost an exact clone of the default media browser view. They all have the a tags.
Comment #22
redndahead CreditAttribution: redndahead commentedDo you have a data-fid attribute? Maybe we can just search for that.
Comment #23
damontgomery CreditAttribution: damontgomery commentedOk, we still had file_styles enabled and it is causing issues. It messes up the wrapper anchors.
I think there was some documentation about disabling it, but we migrated from Media 1.x which was the cause of the issue. Is there some way to notify users to disable that old module if it hasn't already been done. If it has, sorry.
I think we should be good with #18!
Now I have to see if any styles broke without File Styles.
Comment #24
redndahead CreditAttribution: redndahead commentedWe can remove the explicit a in the search. So it becomes
I think the only issue with that is it may be a bit slower. Not that it really makes much of a difference in this case. That would let it be any tag with the data-fid attribute.
Would that have fixed your issue pandaeskimo?
Comment #25
fangel CreditAttribution: fangel commented(Note, I'm using jQuery Update to get jQuery 1.7, so not sure if this is available in the bundled jQuery) Why not use
.data('fid')
instead of using.attr('data-fid')
?Comment #26
Dave Reidusing .data('fid') is available since jQuery 1.4.3 and Drupal 7 uses 1.4.4 by default so that's clear for us to use as far as I know.
Comment #27
damontgomery CreditAttribution: damontgomery commentedNo, it wouldn't have fixed the problem since there was no wrapper. I think we can ignore my issue as a result of having File Styles which should be disabled when switching from Media 1.x to 2.x. We are good to go with the previous patches I think!
Thanks.
Comment #28
redndahead CreditAttribution: redndahead commentedOk here is the updated patch.
Comment #29
redndahead CreditAttribution: redndahead commentedNeeded to update the patch. It seems that data returns numeric value and the settings is a string. Fixed it by not being so strict on the type in the comparison.
Comment #30
fangel CreditAttribution: fangel commentedFWIW, I ran into a problem not having the
array_values
call there, so it's very much so needed. After AJAX-reloading one of the views in the tabs, it would send back the new files as a hash-map style object literal, which the AJAX-settings-merger didn't want deal with (merging a object into an array), so it justDrupal.settings.media.files
with the new object, instead of appending to the array.This broke both the foreach-code to identify the selected file, and all other tabs as their file-infomation had been thrown away in the bad merge.
With
array_values
there, it merges the new settings with the old, and everything keeps working.Comment #31
redndahead CreditAttribution: redndahead commentedSince the current patch has array_values there can you verify that it works for you?
Comment #32
fangel CreditAttribution: fangel commentedIt looks good, but I wont be back at work till Monday, so I'll test the final patch out against our production systems then and report back.
Comment #33
fangel CreditAttribution: fangel commentedYes, the patch in #29 works like a charm.
Comment #34
Dave ReidOk I've committed #29 to 7.x-2.x with a couple of small changes after doing extensive testing with both one or two Views media browser plugin tabs enabled. Thanks everyone for your help on this issue.
http://drupalcode.org/project/media.git/commit/f0bfc42
Comment #36
mrosas CreditAttribution: mrosas commentedI'm still seeing this error after upgrading from 7.x-2.0-unstable5 to 7.x-2.0-unstable6 (which contains the patch in #29). Cache has been cleared and I also tried disabling the "My Files" tab to see if it would work with only the "View Library" view enabled. Would love any insight on possible other conflicts. Happy to provide more configuration information -- let me know what would be helpful.
Comment #37
Jackinloadup CreditAttribution: Jackinloadup commented@mrosas are you using media_youtube? There was a bug in in recently that replicates the same issue described here.
#1627404: View Library submit button broken when media youtube is installed
if you are using media_youtube update to the lastest dev version.
Comment #37.0
Jackinloadup CreditAttribution: Jackinloadup commented-