Media Wysiwyg module uses a variable called 'media_wysiwyg_wysiwyg_allowed_attributes' to define a list of allowed attributes (for example, class, style) which can be applied to media assets attached to content via the wysiwyg.

See https://drupal.org/comment/8138423#comment-8138423 for the full details of this decision.

Although media_wysiwyg does include a function with a default list, it does not actually call this function during install.

function _media_wysiwyg_wysiwyg_allowed_attributes() {
  return array(
    'alt',
    'title',
    'height',
    'width',
    'hspace',
    'vspace',
    'border',
    'align',
    'style',
    'class',
    'id',
    'usemap',
    'data-picture-group',
    'data-picture-align',
  );
}

Because media_wysiwyg.filter.js uses this variable and strips out any attributes which aren't explicitly declared, the JS will actually strip out any attributes you try to attach to the element as you save the node (or as you enable/disable rich text in the wysiwyg). It's pretty confusing for content editors. It's also confusing for site admins, because they have set similar variables in wysiwyg settings, text formats, etc.

I'll submit a patch which sets this variable inside hook_install().

UPDATE

It appears that the variable media_wysiwyg_wysiwyg_allowed_attributes is unused aside from the codebase; there doesn't appear to be any way to set or alter this variable via a gui anywhere. Both instances of variable_get('media_wysiwyg_wysiwyg_allowed_attributes') use the same default function to define the variable; it makes sense to simply remove the variable and always use the default allowed_attributes.

Does anyone know any reason not to remove this variable?

CommentFileSizeAuthor
#8 media-media_wysiwyg_wysiwyg_allowed_attributes_UI-2168291-8.patch901 bytesParisLiakos
#4 media-wysiwyg-remove-allowed-attributes-var-2168291-4.patch2.38 KBkaidjohnson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sheldonkreger’s picture

sheldonkreger CreditAttribution: sheldonkreger commented
Issue summary: View changes
kaidjohnson’s picture

kaidjohnson CreditAttribution: kaidjohnson commented
Status: Needs work » Closed (works as designed)

There is no need to set the variable at install. The javascript whitelist is set by calling

variable_get('media_wysiwyg_wysiwyg_allowed_attributes', _media_wysiwyg_wysiwyg_allowed_attributes_default());

media_wysiwyg.module:186

If the variable media_wysiwyg_wysiwyg_allowed_attributes is not set, the whitelist is generated using the default array. If your media_wysiwyg_wysiwyg_allowed_attributes is set to something undesirable, then it needs to be updated; how it was set in the first place isn't entirely clear to me, as nothing explicitly writes to that variable. You would be in the same state you're in now even if the variable was set during install, as it appears that your db has this variable configured to a different value than default. It's handled this way to prevent unneeded bloating of the variables table. I believe this functionality was last updated via #2126661: Wysiwyg -- allowed_attributes() in media.filter.js does not properly sync with settings from the php.

If, according to https://drupal.org/comment/8345947#comment-8345947, you're stuck at

$ drush vget wysiwyg_allowed_attributes media_wysiwyg_wysiwyg_allowed_attributes: 
array (
  0 => 'class',
)

Then you should probably just

$ drush vdel media_wysiwyg_wysiwyg_allowed_attributes

There shouldn't even be a variable keyed wysiwyg_allowed_attributes -- that's only the reference to the media_wysiwyg_wysiwyg_allowed_attributes in the javascript.

If you can shine some light on how your media_wysiwyg_wysiwyg_allowed_attributes variable was set in the first place, it would be useful to know. I can't find anything in the code that would be setting that variable. It's meant to be maintained by the code and only the code, not users.

sheldonkreger’s picture

sheldonkreger CreditAttribution: sheldonkreger commented

There shouldn't even be a variable keyed wysiwyg_allowed_attributes -- that's only the reference to the media_wysiwyg_wysiwyg_allowed_attributes in the javascript

Sorry for the confusion. Drush actually returns media_wysiwyg_wysiwyg_allowed_attributes when I do 'drush vget wysiwyg_allowed_attributes' . . . Just being lazy typing.

If you can shine some light on how your media_wysiwyg_wysiwyg_allowed_attributes variable was set in the first place, it would be useful to know. I can't find anything in the code that would be setting that variable. It's meant to be maintained by the code and only the code, not users.

This is where the confusion lies, for me. I don't see this variable being set anywhere (normally that would be in the .install file). Media_wysiwyg.install has 'variable_del('media_wysiwyg_wysiwyg_allowed_attributes');' but nowhere do I see this being set.

I see that all of the references to this variable actually call 

variable_get('media_wysiwyg_wysiwyg_allowed_attributes', _media_wysiwyg_wysiwyg_allowed_attributes_default());

This looks for the variable in the DB first, then looks at the default list defined in the module code if it is not set.

Since this variable is new and unique to this module, my argument is that we should probably do this inside media_wysiwyg.install:

variable_set('media_wysiwyg_wysiwyg_allowed_attributes', _media_wysiwyg_wysiwyg_allowed_attributes_default());

The other option is to have the code always call the list defined variables in the module code itself - and not have any database level variable at all. I don't see any reason to do both, although I'm open to opinions.

kaidjohnson’s picture

kaidjohnson CreditAttribution: kaidjohnson commented
Title: Variable 'media_wysiwyg_wysiwyg_allowed_attributes' is not set in install » Remove unused variable: 'media_wysiwyg_wysiwyg_allowed_attributes'
Assigned: Unassigned » kaidjohnson
Issue summary: View changes
Status: Closed (works as designed) » Needs review
FileSize
media-wysiwyg-remove-allowed-attributes-var-2168291-4.patch2.38 KB

@sheldonkreger - yeah, that sounds right. I vote for just dropping the variable handling. I've updated this thread accordingly and propose the following patch.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Closed (works as designed)

The fact this is a variable, it makes overriding easier..removing the variable would mean that for one to customize this list has to hack _media_wysiwyg_wysiwyg_allowed_attributes_default().

So unless we have an alter hook or something, removing this variable will just cause a regression:)

kaidjohnson’s picture

kaidjohnson CreditAttribution: kaidjohnson commented
Assigned: kaidjohnson » Unassigned
Status: Closed (works as designed) » Active

@ParisLiakos - I realize that removing this variable makes it harder to manage the config; however, with the current setup, one still has to write code in order to populate the variable. So if we don't drop it, should there be a gui config option somewhere to be able to manage it? The alternative would be to add an alter hook in _media_wysiwyg_wysiwyg_allowed_attributes_default() and probably rename that to simply _media_wysiwyg_wysiwyg_allowed_attributes().

Which path makes more sense to go down?

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: Remove unused variable: 'media_wysiwyg_wysiwyg_allowed_attributes' » Make 'media_wysiwyg_wysiwyg_allowed_attributes' configurable from UI
Category: Bug report » Task

yes, good point there;) i would prefer this path^. lets rename this issue to reflect this

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Active » Needs review
FileSize
media-media_wysiwyg_wysiwyg_allowed_attributes_UI-2168291-8.patch901 bytes
LittleRedHen’s picture

LittleRedHen CreditAttribution: LittleRedHen commented
Related issues: +#2748663: Alter hook for wysiwyg allowed attributes

This patch means that the media_wysiwyg_wysiwyg_allowed_attributes variable value is a string of space-separated attribute names, which is not what the various functions that use it are expecting.

The issue at https://www.drupal.org/node/2748663 implements the other suggested solution to have an alter hook instead: you probably only need one or the other.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

@LittleRedHen seeing as we've gone with your method
close this as 'closed outdated' ? or duplicate of #2748663: Alter hook for wysiwyg allowed attributes ?

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs review » Closed (outdated)

Closing:
seems to me this issue is no longer relevant.

if you still are concerned about this issue, open a new issue describing (in detail) the use case and add a reference to this closed issue.