Problem/Motivation
Currently all fields that the file entity has are added to the wysiwyg popup. As evidenced in this issue #2062365: Several types of fields are not being handled correctly when file is embedded in the WYSIWYG tag fields don't work because they should be an array and are stored as a string.
Proposed resolution
We discussed rolling back the ability to override the fields in the wysiwyg functionality, but decided that a better solution would be to simply add a whitelist of field types that we know will work in overrides.
Remaining tasks
User interface changes
API changes
Original report by @mirzu
Comment | File | Size | Author |
---|---|---|---|
#109 | Screenshot 2017-02-28 22.51.43.png | 67.47 KB | andyg5000 |
#92 | media-wysiwyg_override_whitelist-2062721-91.patch | 11.57 KB | Anas_maw |
#89 | media-wysiwyg_override_whitelist-2062721-89.patch | 11.5 KB | rooby |
#89 | interdiff-2062721-83-89.txt | 645 bytes | rooby |
#83 | media-wysiwyg-override-white-list-2062721-83.patch | 11.5 KB | mkhamash |
Comments
Comment #1
mirzu CreditAttribution: mirzu commentedHere's a first pass at a patch.
Comment #2
mirzu CreditAttribution: mirzu commentedComment #3
mirzu CreditAttribution: mirzu commentedComment #4
mirzu CreditAttribution: mirzu commentedAdded a variable get and named things better.
Comment #5
genjohnson CreditAttribution: genjohnson commentedThe patch in #4 looks good and works for me.
Comment #6
jvandyk CreditAttribution: jvandyk commentedPatch refers to both "overridden" and "overwritten". These are different things. The description to "Override in WYSIWYG" is unclear. What does checking this checkbox actually do? (I think it stores attribute-value pairs into the text that the button inserts into the field, to be interpreted by the filter.
Comment #7
mirzu CreditAttribution: mirzu commentedAdded update hook, because those are always fun to write.
Comment #8
mirzu CreditAttribution: mirzu commentedRerolled patch with brain on.
Comment #9
mirzu CreditAttribution: mirzu commentedfixed some spelling and stuff.
Comment #10
mirzu CreditAttribution: mirzu commentedI shouldn't be allowed to spell things.
Comment #11
mirzu CreditAttribution: mirzu commentedChanged the way that the filter chooses fields to include.
Comment #12
mirzu CreditAttribution: mirzu commentedAdded an update to the install that will make the fields show up as expected when media is installed.
Comment #13
mirzu CreditAttribution: mirzu commentedI think this handles install and update in a sane way now. Definitely needs testing. It would be extra nice if anyone who has an existing site with lots of media images would test out the update.
Comment #14
dmducc CreditAttribution: dmducc commentedOK...this maybe be me patching against the wrong version of the module (I used: drush dl media-7.x-2.x-dev)
But it seems that media_update_7213() was already used for the following (line 1024):
I changed to media_update_7219() and I was able to use this patch to great success.
I received the error message when trying to add images to a Taxonomy Term description, and then render that description through a view in a custom page.
I did reload a new test image, but might have been able to resave the tax term instead.
dmduke
Comment #15
bneil CreditAttribution: bneil commentedHere's a patch that changes the update to media_update_7219().
In my testing, I noticed that on install, text and text_long fields (tested with alt/title on image type) are not overridable by default without saving the field's form first.
Comment #16
bneil CreditAttribution: bneil commentedRenamed variable wysiwyg_override_field_types to media__wysiwyg_override_field_types to follow other media variable naming scheme.
Added a variable_del for the new variable in media_uninstall().
Comment #18
bneil CreditAttribution: bneil commentedLeft out the update hook on the earlier patch.
Comment #19
aaron CreditAttribution: aaron commentedThis should be a translated string.
Comment #20
bneil CreditAttribution: bneil commentedComment #21
bneil CreditAttribution: bneil commentedAdding beta blocker tag.
Comment #22
lmorgan923 CreditAttribution: lmorgan923 commentedI'm really new to this and have only the beginning of a glimmer of an idea of what I'm doing, but I just ran the patch in #20, and after running the update script, got this message:
The following updates returned messages
media module
Update #7219 - Updated the following field instances: field_tags so they can't be overridden when the file is inserted in the WYSIWYG. Updated the following fields field_file_image_alt_text, field_file_image_title_text so that they continue to show up when a file is inserted.
So it looks to me like the patch ran (plus the messages in my Terminal window were all positive). However, I'm still getting the error message that sent me out looking for a fix, which is: Warning: Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() (line 1666 of /home/mysitemodules/taxonomy/taxonomy.module) plus the same warning for line 1589.
I understand that this issue isn't closed, but - and here's my newbie question - should I have run any of the earlier patches as well? I'm assuming no, that each patch incorporates the earlier patches, but I'd hate to be wrong and miss something that might help.
Comment #22.0
lmorgan923 CreditAttribution: lmorgan923 commentedchanged link to issue number
Comment #23
sheldonkreger CreditAttribution: sheldonkreger commentedPatch in #20 is working for me.
Comment #24
gmclelland CreditAttribution: gmclelland commented20: media-wysiwyg-override-white-list-2062721-20.patch queued for re-testing.
Comment #27
gmclelland CreditAttribution: gmclelland commentedPossible duplicate of this issue #2130847: Allow any field type to be overridden in media
Comment #28
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedthe patch no longer applies. there is no includes/media.filter.inc file in the newest media 7.2 dev module
Comment #29
blacklabel_tom CreditAttribution: blacklabel_tom commentedIs this issue still 7.x-2.0 beta blocker?
Cheers
Tom
Comment #30
pbuyle CreditAttribution: pbuyle commentedWYSIWYG integration has been moved to a separated Media WYSIWYG module (#2142571: Spin-off WYSIWYG support in dedicated module / project). The patch needs to be rerolled for it.
Note: as of today, the WYSIWYG related part of
media_form_field_ui_field_edit_form_alter()
has not been moved to the media_wysiwyg module.Comment #31
seworthi CreditAttribution: seworthi commentedRerolled patch against current dev.
Added config option to "Media browser settings" to select field type from available types in system.
Changed default setting on field config form from TRUE to FALSE.
Comment #32
seworthi CreditAttribution: seworthi commentedComment #34
ann b CreditAttribution: ann b commentedI don't have a tags field (field_tags) defined for any of my file types, but the Media Browser Plus module does add a term reference field called field_folder. I'm getting the following errors after embedding files using the Add Media button in the WYSIWYG:
Warning: Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() (line 1666 of .../modules/taxonomy/taxonomy.module).
Warning: Invalid argument supplied for foreach() in taxonomy_field_formatter_view() (line 1589 of .../modules/taxonomy/taxonomy.module).
Do I need to override these field formatter hooks to add field_folder to the white list?
Test Server Versions:
Drupal Core 7.28
Media Module 7.x-2.0-alpha3
Media Browser Plus for Media 2.x 7.x-3.0-beta3
Thank you,
Ann
Comment #35
bneil CreditAttribution: bneil commentedAnn,
You can eliminate that warning by editing the field on each file type that has the field and unchecking "Override in WYSIWYG". Term reference fields are not supported as overridable in the WYSIWYG. This issue is about providing a whitelist of field types which are allowed by default instead of all of them.
Comment #36
ann b CreditAttribution: ann b commentedThanks @bneil. The warning messages no longer appear after unchecking the Override in WYSIWYG checkbox, just as you said. Sorry for the noise.
Comment #37
heddnClosed #2130847: Allow any field type to be overridden in media as duplicate.
Comment #38
heddnThe previous patch in #2062721-31: Add a white list of file fields that can be overwritten when the file is added in the wysiwyg was originally written prior to media_wysiwyg getting split into a sub-module. The patch makes the needed step to move all the functionality into the sub-module. It also adds a couple things that were missing.
Comment #40
heddnClosed duplicate: #2036495: Alt and title: default values and contect values
Comment #41
heddnWhile this doesn't explicitly depend on #2325671: file_image formatter ignores alt & title overrides on files, none of the overrides will display unless that gets committed. Let's see how the testbot likes things, but I think this will fail until the file_entity patch gets committed. I think that's why we are experiencing the failed tests.
Comment #43
heddnPostponing until #2325671: file_image formatter ignores alt & title overrides on files lands.
Comment #44
heddnFixes an undefined index issue when viewing media from outside of WYSIWYG.
Comment #45
azinck CreditAttribution: azinck commentedThe approach in #44 means that fields without data stored in them on the entity will not get overridden. Without the patch that functionality works fine.
This is needed in order for the overrides to work properly for fields without data in the DB.
Comment #46
heddnWhile re-rolling the patch and splitting out the functionality into the appropriate place, I seem to have lost that functionality. Updated patch attached.
Comment #47
heddnIgnore #46, the patch and interdiff are all wrong. Try this one instead.
Comment #48
azinck CreditAttribution: azinck commented@heddn: I don't think you need media_wysiwyg_field_attach_view_alter() anymore now that you've restored the override functionality via media_wysiwyg_field_attach_view_alter(). Is there a reason to keep that around?
Comment #49
heddnThis should address #48
Comment #50
heddnFields, alt and title text are all getting pulled in from the overridden macro. What is missing is the height/width dimensions. This updated patch picks up that missing piece.
Comment #51
klokie CreditAttribution: klokie commentedpatch in #50 works for me.
Comment #55
heddnThis still postponed on: #2058303: Displaying embedded media without using the File Entity module's built-in formatter ignores attribute overrides.
Comment #56
bneil CreditAttribution: bneil commentedNo longer postponed as #2058303: Displaying embedded media without using the File Entity module's built-in formatter ignores attribute overrides has been committed.
Comment #59
heddnReroll
Comment #61
heddnTestbot, come and get it.
Comment #62
ben.bunk CreditAttribution: ben.bunk commentedI like the direction of this patch, for us we're looking for a way to override the title field on a document file type and this patch doesn't seem to address that concern. Is there any reason why this only works for images? Would the group be open to making these changes work for other file types?
Comment #63
heddnre: #62
There is no limitation of image or other media type.
Comment #64
svenryen CreditAttribution: svenryen commentedThe patch had to be re-rolled. Here's the updated patch.
Comment #65
heddnNot a clean interdiff, but the main piece is that I added logic to check zero indexed macro code and non-indexed macro code in media_wysiwyg_form_wysiwyg_profile_form_alter()
Comment #68
heddnComment #69
heddnComment #70
Danny EnglanderI tested this patch as I ran into the same issue. However, I got an error:
PHP Fatal error: Cannot redeclare media_wysiwyg_update_7201() (previously declared...
... and that's because the latest dev actually includes another db update with the same name, 7201. I attempted to change this patch to 7202 but I don't think it ended up working in the end. I no longer see "Override in WYSIWYG" in my file entity field settings. The field appears briefly when I upload an image via WYSIWYG but then it goes away. I attempted this again on a brand new install of drupal 7 with minimal modules enabled but I probably did not fix the patch correctly to account for the
hook_update(n)
name collision.Comment #71
azinck CreditAttribution: azinck commentedRe-roll against latest dev to fix the probs Danny Englander pointed out. Full disclosure: I haven't tested this, just updated the update hook name and the install hook to refer to the correct update hook.
Comment #72
vegantriathleteI've installed the -dev version and applied the patch.
The error does go away. However, that's only part of the solution as we do need to be able to specify term references for images embedded in the wysiwyg. Also, whether I check or uncheck Term reference is the media browser configuration the term references still are not available in the media browser in the wysiwyg. But, I think that is what's currently intended. (There is a comment right in the patch that says "// Only single valued fields can be overridden.")
I'm going to go back to the issue that was closed as a duplicate of this one, as I think it needs to be reopened. This issue / patch does not solve that problem.
Comment #73
vegantriathleteSo, I'm happy to mark this RTBC if others are happy with just my confirmation that the patch applies and works as designed.
However, I have reopened #2062365: Several types of fields are not being handled correctly when file is embedded in the WYSIWYG since I think that the ability to override term references should still be addressed. And, in fact, there is a proposed solution in the comments.
Comment #74
Devin Carlson CreditAttribution: Devin Carlson commentedComment #75
stred CreditAttribution: stred commented#71 git rid of the warning when an embedded video is displayed. I did not test the rest... does the job for me, thanks!
Comment #76
hawkeye.twolfUploaded the wrong patch file, ignore this one.
Comment #77
hawkeye.twolfUploaded wrong file, updating patch with minor coding standards fix.
Comment #79
Danny EnglanderI tried the new patches, ran
drush updb
and though I no longer get the error #70, I don't see "If checked, then this field may be overridden in the WYSIWYG editor." when I add a select list to a file. Then it never shows up for any view mode. Gosh, maybe I am totally misunderstanding how this is supposed to work. My work around for now is to simply use a text field with the value I need.Comment #80
Dave ReidI would like to get some extra testing on the latest version (patch in #77) before commit. Moving to release blocker for 2.0 release.
Comment #83
mkhamash CreditAttribution: mkhamash as a volunteer commentedRe-roll against the latest dev 7.x-2.0-beta1+8-dev (2015-Aug-19) [Sha 6b94e2f6c], the overrides to fields are not being saved or applied, but the admin interface for fields override does work as expected.
Disclaimer: I have not properly tested this patch and it may not working due to regression issues, I have tested it with bleeding-edge media + media_ckeditor + ckeditor, with tens of patches so.
Comment #84
mkhamash CreditAttribution: mkhamash as a volunteer commentedThis patch will work greatly with the patch in issue #2352573: CKEditor Media WYSIWYG library.js does not pass entity fields into create_element in case of using ckeditor module + ckeditor >= 4.3, though this need more testing but it looks very promising.
Comment #85
anbarasan.r CreditAttribution: anbarasan.r commentedComment #86
anbarasan.r CreditAttribution: anbarasan.r commentedAttached the PATCH against 7.x-2.0-beta1. It might fail because the PATCH not against the latest dev version. But may be useful who want to apply on their site against 7.x-2.0-beta1.
Comment #88
rooby CreditAttribution: rooby commented@anbarasan.r
You can add do-not-test to your patch file name if you want the testbot to skip your patch.
Like this: media-wysiwyg-override-white-list-2062721-86-do-not-test.patch
Comment #89
rooby CreditAttribution: rooby commentedWhoops I didn't mean to post that interdiff in the previous comment.
The patch in #83 didn't work for me.
I haven't done a full review of the patch but I did find an issue, which this patch fixes.
Comment #90
Anas_maw CreditAttribution: Anas_maw at Vardot commentedReroll to latest dev version.
Comment #92
Anas_maw CreditAttribution: Anas_maw at Vardot commentedRetest patch after fix.
Comment #93
Anas_maw CreditAttribution: Anas_maw at Vardot commentedComment #94
joseph.olstadThis is a 2.0 release blocker. @Anas_maw, have you used this patch on todays dev or beta5?
Comment #96
joseph.olstadthis is now in 7.x-2.x dev
beta6 does NOT have this, but 7.x-2.x dev does...
a 7.x-2.0 release will be tagged probably in a few weeks from now.
Thanks to @heddn, @mirzu, @bneil, @derek.deraps, @Anas_maw, @rooby, @azinck, @mkhamash, @seworthi, @anbarasan.r, @svenryen
Comment #97
ar-jan CreditAttribution: ar-jan as a volunteer commentedNice, thanks all.
That's excellent news!
Comment #98
joseph.olstadHere's a list of the media project 7.x-2.0 release blockers
a few left before 7.x-2.0 , maybe a few weeks might be optimistic, haven't yet had a close enough look at the remaining items.
Comment #99
sylus CreditAttribution: sylus commentedThis patch is causing issues for me with Entity Translation all of a sudden my Behat tests checking for the alt attribute on the French Side of the page are no longer working. I have traced it to this commit, as one commit prior (2.0-beta6) is working as expected.
I will create a new issue shortly.
Comment #101
joseph.olstad@sylus, I must have came accross this problem with alt ant title text in french as well (however only when the image style is used and not 'rendered_file', I created a working core patch but maybe its not necessary. We'd have to confirm the details first though before jumping to conclusions.
Here's the core issue I created and provided a working patch, however if this is caused by contrib (this patch in particular) it would be ideal to fix this in 'media' rather than make a change to 'core'.
Was this resolved for you?
here's the related issue:
#2835135: image formatter needs to handle alt/title from file entities on images for multi language support
Comment #102
joseph.olstadadding core issue as "possibly related"
Comment #103
joseph.olstadthis issue appears to be responsible for the regression described by @sylus.
If my suspicions are correct:
Here's the change in the function that I suspect needs review / refactoring. While this works for a unilingual site, multilingual 'alt' and 'title' are getting the default language values under certain conditions as described in #2835135: image formatter needs to handle alt/title from file entities on images for multi language support .
Comment #104
joseph.olstadPossibly related sections of code.
#1553094: Alt and Title support for Images
Comment #105
joseph.olstad@sylus , on my local system I tested backing off all lines of code related to this issue, I also backed off the core patch that fixes the 'alt' and 'title' regression you mentioned, when my fix was backed off, and all changes related to this issue were backed off, it did not fix the multilingual alt / title text issue.
So currently the only way I'm getting 'alt' and 'title' text is via the core patch #2835135: image formatter needs to handle alt/title from file entities on images for multi language support
@sylus, can you please double check again which git hash was causing the issue you noticed? Open a new issue for it in the 'media' queue, and I'll have a look.
Comment #106
joseph.olstad@sylus , it could be that we're not talking about the same french 'alt' issue.
mine is documented in #2835135
Comment #107
joseph.olstadComment #108
sylus CreditAttribution: sylus commentedHey no problem ^_^
I take a look again on latest media release and see if i can reproduce. I'll also create a new issue with exact repro steps if problem occurs.
Thanks a bunch! :)
Comment #109
andyg5000Comment moved to #2856864: Multi-cardinality reference fields break taxonomy_field_formatter_prepare_view