Follow-up to #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored

Problem/Motivation

It seems that #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored had some unexpected side effects.
Before that fix, only ckeditor plugins' required attributes were added to the allowed html field.
Now, required AND allowed attributes are added to the field so they can appear twice (once with the exclamation mark, once without).

As we weren't filtering on attributes lately I don't think that's a big security issue but that's:
- certainly a regression
- probably not what the filter is expecting
- ugly ;)

Current behavior:

Based on the text editor configuration, these tags have automatically been added: <a href hreflang !href target> <img src alt height width data-entity-type data-entity-uuid data-align data-caption !data-entity-type !data-entity-uuid !src !data-align !data-caption class="align-left align-right">.

Expected behavior:

Based on the text editor configuration, these tags have automatically been added: <a href hreflang target> <img src alt height width data-entity-type data-entity-uuid data-align data-caption class="align-left align-right">.

Proposed resolution

No idea.

Remaining tasks

Find out what's wrong. Fix it. Enjoy.

User interface changes

None.

API changes

None.

Data model changes

None.

RC target triage

This bugfix is not disruptive at all as it only affects a few lines of faulty JS already in RC1.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuaelFr created an issue. See original summary.

Jelle_S’s picture

Status: Active » Needs review
FileSize
2.88 KB

It seems the '!'-notation for attributes is only for the string format notation, not needed when using CKEDITOR.style objects. Patch attached ;-)

nod_’s picture

+          if (imgAttributes[i].substr(0, 1) === '!') {
+            allowedContentDefinition.attributes[imgAttributes[i].substr(1)] = '';
+          }
+          else {
+            allowedContentDefinition.attributes[imgAttributes[i]] = '';
+          }

We could use .replace('!', '') here and get rid of the if/else no?

attiks’s picture

AFAIK an exclamation mark is allowed in a data-* attribute, so I think the safest is as it is in the patch.

nod_’s picture

replace can take a regex, so if it's only about a ! in the first position we can do that too. .replace(/^!/, '');

attiks’s picture

True, either way is fine by me

nod_’s picture

Status: Needs review » Needs work

let's go with that then. less lines can't hurt.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
2.72 KB
nod_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.85 KB
812 bytes

Just fixing eslint errors. Works, no duplicate attributes in the list now.

DuaelFr’s picture

Issue summary: View changes
Issue tags: +rc target triage
Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

This patch does indeed fix the !attribute is whitelisted problem.


But…

Prior to #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored, the act of merely browsing to /admin/config/content/formats/manage/basic_html resulted in no tags being added.

With this follow-up patch for #2579979, we still see:

Based on the text editor configuration, these tags have automatically been added: <a href hreflang target> <img src alt height width data-entity-type data-entity-uuid data-align data-caption class="align-left align-right">.

The things that are being added specifically are:

  • the target attribute on <a>
  • the class="align-left align-right" attribute + attribute values on <img>

Therefore, this is still a net regression.

Wim Leers’s picture

It seems the changes in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored are triggering a bug in CKEditor.

filter.filter_html.admin.js's _calculateAutoAllowedTags() is most definitely only looking at the required tags. So I added a bit of debug output there (console.log(featureName, featureRule);) to see what's going on. Turns out CKEditor is returning ['href', 'target'] as required attributes for <a>. Even though target is only an allowed attributes. It seems they have some sort of object leaking/object reference bug in their parsing of plugins' allowedContent/requiredContent and the mapping of that into their "CKEditor feature rules".

I pinged the CKEditor team for their help.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
1.05 KB

Updated core/profiles/standard/config/install/filter.format.basic_html.yml to be consistent with its enabled editor plugins on install.
That makes browsing to the /admin/config/content/formats/manage/basic_html not resulting in added tags anymore.

Changing the issue status to Needs Review only to trigger the testbot. As told in #12 we need more input from the CKEditor team.

Wim Leers’s picture

#13: that's wrong. Those things should not be whitelisted.

This explains why you said #2578957: (regression) "Open in new window" checkbox does not work anymore was "magically fixed" by #2579979! You want target to be whitelisted. But it's only an allowed thing, not a required thing. If people want to allow that, they need to manually whitelist target. The point is that we only whitelist tags and attributes that are essential (required) for that editor feature to function. target is not essential.

DuaelFr’s picture

So, we might want to remove the "Open in a new window" checkbox from the link Dialog when the attribute is not whitelisted... or totally remove that checkbox and let contrib deal with it.

We cannot expose something to end users by default and assume they know they have to change something in an obscure part of the settings to get it working.

Wim Leers’s picture

First: you're absolutely right of course.

We cannot expose something to end users by default and assume they know they have to change something in an obscure part of the settings to get it working.

We should only expose it if the text format allows it. Just like EditorImageDialog for example only shows the "Align" checkboxes if it's allowed:

    // When Drupal core's filter_align is being used, the text editor may
    // offer the ability to change the alignment.
    if (isset($image_element['data-align']) && $filter_format->filters('filter_align')->status) {

In the case of <a target>, we should do something like

if ($filter_format->getHtmlRestrictions()['allowed']['a'] === TRUE || $filter_format->getHtmlRestrictions()['allowed']['a']['target'] === TRUE) {

… but that probably belongs on #2578957: (regression) "Open in new window" checkbox does not work anymore.

DuaelFr’s picture

I didn't know that thing existed!
So, why did we decide that data-align was so important it had to be enabled by default (in standard profile at least) but link target is not?
We should fix that checkbox for sure but I think the standard profile is the good place to have that kind of thing enabled by default. That last thing should probably live in a follow-up issue, though. ;) Yay ! Chaining issues !

So, to summarize:

  1. we need to fix that allowed tags/attributes being automatically added to the allowed html in this issue
  2. we need to make the target checkbox on the EditorLinkDialog aware of the allowed html tags in #2578957: (regression) "Open in new window" checkbox does not work anymore
  3. we need to discuss about that target attribute (and the related checkbox) being enabled by default in the standard profile in an other follow-up

I'm running out of time. Could you open that last follow-up and reopen the one I closed as duplicate, please?

DuaelFr’s picture

Issue tags: -rc target triage

Thanks a lot! I'll post there tonight or tomorrow.
Removing rc target triage tag while we don't have something that's commitable.

Reinmar’s picture

The patch for https://www.drupal.org/node/2579979 does not seem to be correct, because the code tries to extend one feature with another feature, while (if I remember correctly) some previous version of that patch tried to extend one allowedContent rules with another ones. The previous one made more sense.

The second thing is that using CKEDITOR.style to define allowedContent work, because this is a valid argument of CKEDITOR.filter.allow() but it has a special meaning. All properties (styles, attrs) of such style are treated as required, because only elements having all of these properties are considered matching that style. So I would rather recommend using the object notation of allowed content rules rather than CKEDITOR.style instances.

And the third thing is how to extend one allowed content ruleset with another ruleset. I guess you don't want this to happen:

allowedContent 1: { element: ‘a’, attributes: { ‘href’ } }
allowedContent 2: { element: ‘a’, attributes: { ‘target’ } }

If you now do: CKEDITOR.tools.extend( aC1, aC2, true ) you will have:

{ element: ‘a’, attributes: { ‘target’ } }

Because the whole attributes property of aC1 is overridden.

I guess that you want to merge attributes lists. For that a deep-extend would make more sense, but you can also write a custom extend() function if this will require some special logic.

Few more pieces from what I wrote Wim on Skype:

OK. I’ll need to compare the previous patch that I saw with the latest one. However, I can confirm that if you do this https://gist.github.com/Reinmar/05e61294fec0d282a171 then all (3 :D) attributes are required. That’s because the style makes sense only as a whole (with all its attributes), so when you call editor.filter.allow( someStyle ), then filter will add a rule which requires all the properties of that style.

Also, important thing is that command.requiredContent isn’t registered to the filter. It is only used to verify whether the current ruleset allows a usage of that specific command.

(...)

I’m not sure if this will clarify what I wrote, but CKEDITOR.command implements the CKEDITOR.feature interface. The feature interface contains such properties as allowedContent and requiredContent. Now, depending on how the editor is configured, they are used in a different way:

1. If the ACF works in the automatic mode (the config.allowedContent isn’t defined), then allowedContent rules of the features that are registered to the editor are added to editor.filter. Later on, some plugin may need to know whether usage of a feature that hasn’t been registered is allowed in this editor. To check that we use the feature.requiredContent property and see if it’s satisfied by the registered allowedContent rules.

2. If the ACF works in a custom mode (config.allowedContent is defined), then to register feature we need to check whether it’s allowed, so again, we check whether feature.requiredContent is satisfied by config.allowedContent.

So that’s how requiredContent is used.

Now, editor.filter.allow() accepts many different formats of allowed content rules. There’s the string format, the object format, and CKEDITOR.style instances. You tried to use the last one but CKEDITOR.style has a special meaning. If someone defines e.g. the following style:

{ element: ‘span’, styles: { color: ‘white’, background-color: ‘red’ ) }

and the style is registered to the editor as something that should be allowed, we want to accept this:

<span style=“color: white; bg-color:red”>xxx</span>

But we want to reject this:

<span style=“color: white;”>xxx</span>

Because this element does not match that style definition – it’s not complete so it represents some other feature. Therefore, all properties of a CKEDITOR.style are registered as required. And that’s what you can find later in editor.filter._.allowedContent.

Jelle_S’s picture

Ok, so if I'm not mistaken, we can't use CKEDITOR.style for our use case. The next 'most easily extendable' would be the object format. But, from what I gather, in the object format, the attributes and styles use string notation:

{
  a: {
    attributes: '!href target'
  }
}

Correct me if I'm wrong but I don't think we can do something like:

{
  a: {
    attributes: ['!href', 'target']
  }
}

or

{
  a: {
    attributes: {'!href': '', 'target': ''}
  }
}

Correct?

Reinmar’s picture

Actually you can specify attributes in one string, an array or an object (values must be true): check these tests https://github.com/ckeditor/ckeditor-dev/blob/master/tests/core/filter/a... and this fragment of docs http://docs.ckeditor.com/#!/guide/dev_allowed_content_rules-section-spec...

So if you'll use a purely object format, then doing a deep copy from one object to another would work as extending one style with another.

Jelle_S’s picture

Ok here's a new patch.

According to the docs I can't use the object format for requiredContent, it's either string format or CKEDITOR.style format. But it seems CKEDITOR.style is valid for requiredContent for our use case. @Reinmar can you confirm?

The allowedContent has been converted to the object format.

Not sure if this is because of the patch (but I don't think so) or default behaviour from before the patch:
- Edit the restricted HTML format and select CKEDITOR as editor.
- When I add the image align plugin to the restricted HTML format, nothing is added to the allowed HTML filter.
- Save and configure it again, and under the allowed HTML filter I get the message: "Based on the text editor configuration, these tags have automatically been added: <img data-entity-type data-entity-uuid src data-align data-caption>."
This means you can save the text format with plugins that will not work

I think this would be solved if the plugin.js files are loaded on the config page, regardless of whether or not the plugin is enabled (I have not tested this theory yet, but fact is that they are not loaded when they are not enabled, and are loaded when they aren't).

Just checking if this behaviour is a bug introduced by this patch or the patch in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored.

Wim Leers’s picture

FileSize
1013 bytes

@Reinmar can you confirm?

Pinged Reinmar to confirm that. That sounds very strange/confusing :(


Not sure if this is because of the patch (but I don't think so) or default behaviour from before the patch:

Before which patch? This one, or #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored?

I think this would be solved if the plugin.js files are loaded on the config page, regardless of whether or not the plugin is enabled (I have not tested this theory yet, but fact is that they are not loaded when they are not enabled, and are loaded when they aren't).

That is already the case. That's how this is able to work at all. See \Drupal\ckeditor\Plugin\Editor\CKEditor::settingsForm():

    // Hidden CKEditor instance. We need a hidden CKEditor instance with all
    // plugins enabled, so we can retrieve CKEditor's per-feature metadata (on
    // which tags, attributes, styles and classes are enabled). This metadata is
    // necessary for certain filters' (e.g. the html_filter filter) settings to
    // be updated accordingly.
    // Get a list of all external plugins and their corresponding files.

So I'm not sure I completely comprehend what you're saying here.

Because even in case you go to /admin/config/content/formats/manage/restricted_html (which definitely has no image-related filters: <img> is not even allowed) and then enable CKEditor for the very first time, then you see that /core/modules/ckeditor/js/plugins/drupallink/plugin.js and /core/modules/ckeditor/js/plugins/drupalimage/plugin.js.

EDIT: oh, I see — despite all that, the hidden CKEditor instance still ends up respecting each Drupal CKE plugin's isEnabled() method. I attached an interdiff with the fix for that.

Jelle_S’s picture

Pinged Reinmar to confirm that. That sounds very strange/confusing :(

I agree. BTW, here's the doc link where I found it:
http://docs.ckeditor.com/#!/api/CKEDITOR.filter.contentRule

This is a simplified version of the CKEDITOR.filter.allowedContentRules type. It may contain only one element and its styles, classes, and attributes. Only the string format and a CKEDITOR.style instances are accepted. Required properties are not allowed in this format.

Before which patch? This one, or #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored?

Either one, if the behaviour has changed at all.

So I'm not sure I completely comprehend what you're saying here.

Because even in case you go to /admin/config/content/formats/manage/restricted_html (which definitely has no image-related filters: <img> is not even allowed) and then enable CKEditor for the very first time, then you see that /core/modules/ckeditor/js/plugins/drupallink/plugin.js and /core/modules/ckeditor/js/plugins/drupalimage/plugin.js.

Yes, but the drupalimagecaption is not loaded I think, so I assume with the interdiff it is?

Should the interdiff be part of this patch or a separate issue?

Reinmar’s picture

ccording to the docs I can't use the object format for requiredContent, it's either string format or CKEDITOR.style format. But it seems CKEDITOR.style is valid for requiredContent for our use case. @Reinmar can you confirm?

Yes, this is true. There's a big difference between allowedContent (filter.allow()) and requiredContent (filter.check()), so both these parameters are handled by a totally different code. As I look on the code now, it might be doable to support object format in requiredContent, but that's mostly thanks to refactoring that we've done after first implementation of filter.check(). Previously it would require a lot of code, so we didn't want to bloat the editor with it as it wasn't necessary so far.

BTW. I thought about the required content in case of Drupal. As I mentioned in #20 it's used by the editor to figure out whether current filter's settings allow for enabling a given feature. For instance, an image feature may allow img[src,alt,width,height](someclasses,here), but in fact to work it requires just this img[src,alt]. So the two things are:

  • CKEditor's filter needs to match elements, attributes, classes and styles. AFAIR Drupal's HTML filter is configured by a list of elements because the list of attributes is predefined (all except sth). It means that if Drupal configured the editor based on Drupal's HTML filter's configuration that already allows for example img[*](*). Hence, the requiredContent will be satisfied no matter what some feature will add to it (I mean extending one features reqContent with another features reqContent).
  • As I imagine, if you have let's say the drupalimage feature and drupalimagestyles which can extend drupalimage, then most of the time the base feature defines a sufficient list elements. Drupaliamagestyles would only require some more attributes. Since Drupal configures CKEditor to allow elements with all attributes, you can assume that drupalimagestyles will be allowed as well. Hence, I don't think you need to worry about extending requiredContent of a base feature.
Reinmar’s picture

I've been looking at https://www.drupal.org/files/issues/core-js-ckeditor-2585173-23.patch and I don't understand one thing – why do you need to split classes, styles and attributes like this:

         var imgAttributes = widgetDefinition.allowedContent.img.attributes.split(/\s*,\s*/);

If I understand that piece of code correctly widget definition is a definition of drupalimage, which is your plugin. Can't you define allowed content in an object format from the beginning and avoid that work on strings?

xjm’s picture

Priority: Normal » Major
Issue tags: +rc target

This might even be critical, but @alexpott and I agreed that this should be an rc target at least, assuming the eventual fix does not introduce unexpected disruption.

Wim Leers’s picture

Title: (regression) Allowed HTML tags field badly filled » (regression) "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI
Component: editor.module » ckeditor.module
Assigned: Unassigned » Wim Leers

I worked on determining the correct priority for this issue. Clarified the title and moved to the right component. Next: move this patch forward.


If you follow these steps:

  1. Install D8
  2. Go to /admin/config/content/formats/manage/basic_html
  3. Click Save
  4. Now the allowed_html setting is strongly polluted: it contains lots of exclamation mark-prefixed attributes. :( :( :( In particular, the <img> tag gets all the data- attributes added again, but now prefixed with exclamation marks. The question is: does this break things?
  5. Go to /node/add/article. Note that everything still works perfectly. Look at drupalSettings.editor.formats.basic_html.editorSettings.allowedContent.img.attributes. Be utterly surprised that this actually does NOT contain any of the exclamation mark-prefixed attributes!
  6. Set a breakpoint in FilterHtml::getHTMLRestrictions(), notice that because this uses DOMDocument to parse the allowed_html setting, it turns out that it automatically ignores/skips the invalid (exclamation mark-prefixed) HTML attributes! Three hurrays for DOMDocument!

Conclusion: through sheer luck, this does not negatively impact the end user; it only negatively impacts:

  1. people using the the Text Filter & Editor configuration UI
  2. the configuration in the filter.format.*.yml files

If it were just point 1, this would be normal. With point 2, I think this is major. If this also affected end users (i.e. people using CKEditor/text formats), then I think this would be critical.

Wim Leers’s picture

First, a straight reroll of #23, with conflicts resolved. (This conflicted with #2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats.)

Next: working on making that better, taking into account all the feedback from CKEditor lead developer @Reinmar.

Wim Leers’s picture

#26:

It means that if Drupal configured the editor based on Drupal's HTML filter's configuration that already allows for example img[*](*).

This used to be true. But this changed shortly before we entered the RC phase. See #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default. Since then, no attributes are whitelisted by default. You need to explicitly whitelist all attributes you want to use on tags.

#27: I also don't understand that.

#28: Thanks!


Stay tuned.

Wim Leers’s picture

#27: oh, it's because image2 uses the string format, and drupalimage wants to extend image2.

Wim Leers’s picture

Fixing one small oversight in #30's rebaing that caused a JS error.

Wim Leers’s picture

So the actual root cause of the problem here is that CKEditor plugins have poor extensibility. Drupal has a very strong need for multiple things extending the same original thing. For example, a form can be altered by multiple Drupal modules. And applied to CKEditor, that means that multiple Drupal modules need to be able to extend the functionality of the same CKEditor plugin, which then also means adding more required and/or allowed attributes.

Let's look at a concrete example. Let's say we have an "Crazy Image Effect" module. This module allows you to apply a crazy image effect to any image. To achieve this, it has a filter that looks at <img> tags with a data-crazy-image-effect attribute. Hence this attribute needs to be allowed for this to work. Hence we need the data-crazy-image-effect attribute to be in requiredContent. The end result is then that the module can alter the EditorImageDialog form, that form then is able to communicate the user-selected value for data-crazy-image-effect to CKEditor, and CKEditor doesn't strip it because the attribute is whitelisted in the text format's allowedContent, because it was marked as required for that plugin.

The changes made in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored were an effort to make this kind of extensibility actually feasible without having to parse values in a CKEditor plugin's requiredContent and allowedContent. If CKEditor would improve the extensibility out of the box, we wouldn't have to go through all this :) AFAICT, CKEditor currently only cares about one kind of extensibility: extend by subclassing. But subclassing only allows one thing to extend the base thing, and Drupal needs the ability for multiple things to extend it.

Wim Leers’s picture

  1. Going back to what we had before #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored is not an option, because:
    • drupalimage had to override image2: widgetDefinition.requiredContent = 'img[alt,src,width,height,data-entity-type,data-entity-uuid]';
    • and drupalimpagecaption had to override drupalimage: widgetDefinition.requiredContent = 'img[alt,src,width,height,data-entity-type,data-entity-uuid,data-align,data-caption]';

    This is exactly the "designed for single extensibility" problem described in more detail in #34.

  2. Therefore going with the object format for allowedContent and a CKEDITOR.style instance for requiredContent, per #26.
  3. But, instead of mixing the "mapping from string format to appropriate more extensible format" and the "update to reflect drupalimage's needs" concerns, now doing those separately. This makes things much clearer. Also including ample documentation (including some that was lost in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored) and justification, to help avoid developers having to go through this entire issue, because this is definitely far more confusing than it should be. And is hence a thing to improve in a future CKEditor release.
  4. Also updated drupallink's allowedContent to include classes: {}, just like drupalimage, so that it is just consistently extensible like drupalimage.
Wim Leers’s picture

This fixes an eslint error that #23 introduced.

Wim Leers’s picture

Title: (regression) "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI » [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI
Wim Leers’s picture

Issue tags: +Needs manual testing

#23:

Not sure if this is because of the patch (but I don't think so) or default behaviour from before the patch:

I replied with an explanation and suggested interdiff in #24. But my fix was wrong. Opened #2606496: Automatically allowed automatically attributes may be incomplete if they are for a CKEditor plugin that is enabled depending on a filter to fix it, and indicates possible solutions.

Hence this issue is now fully focused on just the issue title, and #36 fully fixes it. We need manual testing and confirmation from the CKEditor developers.

DuaelFr’s picture

Issue tags: -Needs manual testing

I did some manual testing on images and links, according to everything that @Wim Leers said since #29.
The patch from #36 answers all the needs exposed here.
I just have a tiny comment that does not affect the fix. (It's RTBC-able for me)

+++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
@@ -29,41 +29,48 @@
+        widgetDefinition.requiredContent = new CKEDITOR.style(requiredContent);

May we have a small comment that explain we have to rebuild the entire object because we do not have any setter? I have to read the CKEDITOR code to understand why this was needed.

-----

I plan to use that patch logic this week on both my D8 Editor File upload module and a client site that need to be able to add the title and target attributes on their links.

Wim Leers’s picture

Glad to hear that you agree this answers all raised problems/needs!

May we have a small comment that explain we have to rebuild the entire object because we do not have any setter? I have to read the CKEDITOR code to understand why this was needed.

Of course. Will do so in the next reroll.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
7.09 KB
1.74 KB

Done.

Unassigning, because this is now blocked on the CKEditor team.

DuaelFr’s picture

#41 is great. Thank you, again :)
That's RTBC for me, let's wait for the CKTeam update.

Reinmar’s picture

  • Do you need to set widgetDefinition.allowedContent and widgetDefinition.requiredContent twice in drupalimage/plugin.js?
    Couldn't you set them once and comment which attributes come from image2 and which are Drupal specific?
    Of course it won't cause any troubles if you set them twice, but code size grows :)
  • Are data-entity-* attributes really required? Issn't requiring them breaking pasting content from outside the editor?
    AFAIU, images without those attributes will be filtered out.
  • Is data-align a required attribute? Can't you have an unaligned captioned image?
  • Does drupalunlink command need the allowedContent setting? It does not create anything.

Other than that the patch looks great.

Wim Leers’s picture

Do you need to set widgetDefinition.allowedContent and widgetDefinition.requiredContent twice in drupalimage/plugin.js?
Couldn't you set them once and comment which attributes come from image2 and which are Drupal specific?
Of course it won't cause any troubles if you set them twice, but code size grows :)

It's possible to merge them, but IMO separating the concerns makes the code much easier to understand. It also means that when people want to extend drupalimage, they see an example right there of how to do it correctly.

Are data-entity-* attributes really required? Issn't requiring them breaking pasting content from outside the editor?
AFAIU, images without those attributes will be filtered out.

No, that attribute is not required to be present on every image. But the plugin/widget does require those attributes to be allowed; without them, it wouldn't be possible to upload images and track where they are being used. Note that the default text format (Basic HTML) does have the Restrict images to this site filter enabled, which means that it is in fact impossible to hotlink images from other domains.

So, it's actually kind of true that those attributes are required for images. But really, what this is meant to convey, is that for this plugin to function, it needs those attributes to be allowed. Is this the wrong way of defining that?

Also, there's nothing new here, it's been like this for years,

Related improvement targeted for Drupal 8.1.0: #2510394: [drupalImage] Setting to still allow linking to an image by URL even when uploads are enabled.

Is data-align a required attribute? Can't you have an unaligned captioned image?

And vice versa, you can also have an aligned uncaptioned image. The problem here is that we actually want to express "it is required to have either of these attributes". The drupalimagecaption plugin should actually be split up into drupalimagecaption and drupalimagealign, and both should extend drupalimage. But that's something for 8.1, unless you feel that is essential?

Does drupalunlink command need the allowedContent setting? It does not create anything.

Hah! We've had this since the very issue where we introduced the drupallink plugin: #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms. This went unnoticed all that time, including by @wwalc, who reviewed that issue. Removed, thanks!

Other than that the patch looks great.

Yay!


Awaiting your response!

Reinmar’s picture

Regarding data-entity-* and data-align – it's up to you whether you want to make them required or not. Defining them as required will limit what the users can do. But if your UI is consistent with that (e.g. that it requires choosing some alignment if you chose caption), then it's totally OK.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Okay, @Reinmar is clearly +1 to the overall changes here.

His concerns are mostly regarding pre-existing things, which are A) at least partially correct, B) out of scope to resolve here, C) are too late to change now anyway given we're approaching release. Let's improve those things in Drupal 8.1.x.


Therefore, per #39, #42, #43, #44 and #45, this is RTBC.

Wim Leers’s picture

#44 + #45:

Does drupalunlink command need the allowedContent setting? It does not create anything.

Hah! We've had this since the very issue where we introduced the drupallink plugin: #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms. This went unnoticed all that time, including by @wwalc, who reviewed that issue. Removed, thanks!

Opened separate issue for this, since that is a completely harmless thing (that should nevertheless be tested separately to ensure it doesn't cause regressions), and this issue is a very important regression. Then this issue can go in without the need for a new round of manual testing.
See #2607454: Remove allowedContent for drupalunlink command.

Wim Leers’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Also given the amount of change in this area atm a new set of manual testing would probably be good.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +Needs manual testing
FileSize
7.12 KB

+1.

Here's a straight reroll. DuaelFr will be doing manual testing in an hour or so. I already did brief manual testing; still working as in #41 AFAICT.

attiks’s picture

I reviewed the patch (not tested) and it looks good, code is even more readable, great job!

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing +Needs change record

I did a lot of manual testing with all Core's plugins and my own one.

All the Core plugins are perfectly working. No more extra buggy attributes added to the list on page load.
I had to update my module to be able to see the issue fixed with a custom plugin so we might need a quick change record.

You did a great job :)

Wim Leers’s picture

Issue tags: -Needs change record

Thank you very much for your extensive testing!

I disagree we need a change record. No APIs are changed in this patch. What this patch does, is fixing Drupal's own CKEditor plugins to comply with CKEditor APIs. i.e. we are using CKEditor APIs incorrectly in HEAD, as CKEditor lead developer @Reinmar explained in #26.

The patch therefore merely fixes core, and documents why allowedContent and requiredContent use different formats:

+++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
@@ -29,41 +29,51 @@
+        // Mapped from image2's requiredContent: "img[src,alt]". This does not
+        // use the object format unlike above, but a CKEDITOR.style instance,
+        // because requiredContent does not support the object format.
+        // @see https://www.drupal.org/node/2585173#comment-10456981

(That link points to #26.)

Therefore the change you made (http://cgit.drupalcode.org/editor_file/commit/?id=8238733e057f6eb1c72397...) is just another change to comply with CKEditor's limitations. That is in no way a Drupal API. You had updated your module's CKEditor plugin to reflect recent changes in core, and in doing so, you inherited core's bug. Any future Drupal modules including a CKEditor plugin will look at core and see everything explained and documented in the correct way and will hence not make the same mistake.


The real problem is that CKEditor doesn't validate the incoming data: if it would be throwing an exception, none of this would ever have happened. I opened a ticket for that: http://dev.ckeditor.com/ticket/13898.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

YAY! Thanks, all. Committed/pushed to 8.0.x. Thanks!

  • webchick committed 735dc78 on 8.0.x
    Issue #2585173 by Wim Leers, Jelle_S, DuaelFr, nod_, Reinmar, attiks: [...
Wim Leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.