Update: See comment #18 for some analysis by @wimleers of the cause of this regression.
Needs review by form system maintainers

Work around: Whilst the UI isn't saving the caption and alignment tags, it is possible to align inline images by manually adding the 'data-align' and 'data-caption' attributes to the img tag in the HTML source code.

eg:
<img alt="ALT TAG TEXT" data-align="RIGHT" data-caption="CAPTION TEXT" data-entity-type="FILE" src="/IMAGE.PNG" />

Investigation

$ git bisect start 8.0.0-beta12 8.0.0-beta11
At each step, reinstall Drupal standard, login and try to align an image in a page.

Result : 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7 is the first bad commit

Reverting changes introduced by this commit, the problem comes from the file module.

---------------
Myself and others over the past few days have noticed that no matter how many times you select Align options or try to enable Caption for an image the settings do not work.

I have seen this on someone else's machine (they were trying to blame Bartik on the frontend).
I have tried on my local install and also on Simplytest.me whilst testing a Captions related issue for Bartik.

If you select an option and press save in the modal window the settings do not apply. When you double the click on the image to try again the values are set to the default values, ie. none or unticked.

----------------
Issue summary from one of the dupes #2533738: Image align radio and caption checkbox in CKeditor does not work anymore with more details

Problem/Motivation

In beta11 it was possible to align images uplaoded in the CKEditor by selecting a radio button in the popin. Since beta12 it is not possible anymore.

Investigation

$ git bisect start 8.0.0-beta12 8.0.0-beta11
At each step, reinstall Drupal standard, login and try to align an image in a page.

Result : 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7 is the first bad commit

Reverting changes introduced by this commit, the problem comes from the file module.

Steps to reproduce:

  1. $ git checkout 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7^
  2. Install drupal standard
  3. Login and create a page
  4. Insert an image in the body with align = right
  5. the image is aligned in the editor and in the saved content
  6. $ git checkout 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7
  7. Install drupal standard
  8. Login and create a page
  9. Insert an image in the body with align = right
  10. the image is not aligned in the editor nor in the saved content

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because that feature used to work
Issue priority Major because it's a widely used feature
Prioritized changes The main goal of this issue is to fix a regression

Proposed resolution

No idea.

Remaining tasks

Fix that regression.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

emma.maria’s picture

Title: Ckeditor image options (align and caption) no longer work. » Ckeditor image options (align and caption) are not working.
Wim Leers’s picture

Title: Ckeditor image options (align and caption) are not working. » EditorImageDialog alignment & captioning are not working
Component: ckeditor.module » editor.module
Issue tags: +Needs tests
Related issues: +#2395377: Write integration test for EditorImageDialog

Yep, reproduced. Something broke this :(

It is not a JavaScript problem; it's the server's response that is missing the data-caption and data-align attributes.

This is why we really need #2395377: Write integration test for EditorImageDialog.

Wim Leers’s picture

szeidler’s picture

I found out, that it seems to be related to the $image_element array in /core/modules/editor/src/Form/EditorImageDialog.php.
When I change line 184

if (isset($image_element['data-align']) && $filter_format->filters('filter_align')->status) { 

to

if ($filter_format->filters('filter_align')->status) {

the align feature is working again. Same goes for the caption condition.

I compared the module with drupal-8.0.0-beta10 where the align/caption dialog was working. I recognized, that the buildForm() method is also called when submitting the dialog. That seems not the case in the old beta, when I debugged it correctly.

Does someones has an idea, why that is happening?

davidhernandez’s picture

I just tested this in head. The alt text works, the manual sizing works, but the caption and alignment do nothing.

I agree with Wim; temped to mark this critical.

larowlan’s picture

Assigned: Unassigned » larowlan

Hit this in a demo, working on a test

Wim Leers’s picture

Many thanks!

kattekrab’s picture

Priority: Major » Critical

Yes, I think this should be critical. It's "in your face" functionality that just doesn't work, and a regression, because I'm pretty sure it WAS working fine.

Bumping.

Core crew, feel free to disagree and bump it back.

larowlan’s picture

Priority: Critical » Major

From https://www.drupal.org/node/45111

Critical Bug:
Render the system unusable and have no workaround.
Cause loss of data.
Expose security vulnerabilities.
Cause tests to fail in HEAD on the automated testing platform, since this blocks all other work.
Cause race conditions, database deadlocks etc. for which even code with 100% automated test coverage may be affected

Regressions in functionality are not automatically considered critical. Even if the bug existed before, it should be prioritized according to its impact.

Major Bug:
Bugs that have significant repercussions but do not render the whole system unusable (or have a known workaround) are marked major.

Examples of major bugs:

A non-fatal PHP error, or a PHP error triggered under rare circumstances or affecting only a small percentage of all users.
Test failures in secondary environments (for Drupal 7 and 8 core).

Think that means major is even a stretch

larowlan’s picture

Should be red/green

The last submitted patch, 10: image-caption-2540850.fail_.patch, failed testing.

kattekrab’s picture

Issue summary: View changes
FileSize
103.01 KB

Nicely done @larowlan

Manually tested with simplytest.me - now works as expected.

screengrab:

szeidler’s picture

Thanks for creating the patch @larowlan. Then I identified the right thing. Also I can confirm the caption and alignment is working again out of the dialog window in HEAD.
I'm still a little bit curious why the array properties in $image_elements got lost.

larowlan’s picture

The form is being rebuilt and it uses the cached one from the $form_state instead of from the $user_input

> form is built
> values don't exist
> image_elements is stored in form state without those values
> form is submitted
> old values are used instead of new ones from input

fix makes sure they are stored, even if no caption or alignment

edit: this is wrong - see next comment

larowlan’s picture

Actually, that's not right

> form is built with all elements
> file upload via ajax submits to the original url (this used to submit to system/ajax) but no longer does, so in the new code-path the form is rebuilt
> in that instance those values aren't set (you upload the file first)
> so the fields aren't added to the $form
> then you submit again with all values filled
> form is retrieved from cache without those elements
> $form_builder->doBuildForm moves values from $form_state->input to $form_state->values based on fields present in the retrieved form - but those fields aren't there, so it never makes it from the input values into $form_state->values and is hence lost.
> fix is to make sure the fields are always there (if enabled) rather than only if $image_elements contains them in the incoming post values

So based on that I think #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache was the cause.

larowlan’s picture

Wim Leers’s picture

Thank you so much, @larowlan!

But, like you already wrote in #15 & #16, this is actually wrong:

+++ b/core/modules/editor/src/Form/EditorImageDialog.php
@@ -181,7 +181,7 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor
-    if (isset($image_element['data-align']) && $filter_format->filters('filter_align')->status) {
+    if ($filter_format->filters('filter_align')->status) {
       $form['align'] = array(
         '#title' => $this->t('Align'),
         '#type' => 'radios',
@@ -191,7 +191,7 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor

@@ -191,7 +191,7 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor
           'center' => $this->t('Center'),
           'right' => $this->t('Right'),
         ),
-        '#default_value' => $image_element['data-align'] === '' ? 'none' : $image_element['data-align'],
+        '#default_value' => empty($image_element['data-align']) ? 'none' : $image_element['data-align'],
         '#wrapper_attributes' => array('class' => array('container-inline')),
         '#attributes' => array('class' => array('container-inline')),
         '#parents' => array('attributes', 'data-align'),
@@ -200,11 +200,11 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor

@@ -200,11 +200,11 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor
 
     // When Drupal core's filter_caption is being used, the text editor may
     // offer the ability to in-place edit the image's caption: show a toggle.
-    if (isset($image_element['hasCaption']) && $filter_format->filters('filter_caption')->status) {
+    if ($filter_format->filters('filter_caption')->status) {
       $form['caption'] = array(
         '#title' => $this->t('Caption'),
         '#type' => 'checkbox',
-        '#default_value' => $image_element['hasCaption'] === 'true',
+        '#default_value' => !empty($image_element['hasCaption']) && $image_element['hasCaption'] === 'true',
         '#parents' => array('attributes', 'hasCaption'),

None of these changes should be necessary; only when the data received from the text editor indicates that the text editor supports data-align and/or data-caption, only then, these form items should be shown.

(And in core specifically: when using only the caption filter OR only the align filter, then the text editor will also only support either, and this form should match that. That's also how we can expand the test coverage to check that.)

Wim Leers’s picture

So, here's another complete analysis. That will hopefully allow a form system maintainer to help us out here.

Step 1: open image dialog

Confirmed that the initial building of the form works correctly. The values the form receives are the ones that Drupal.editors.ckeditor.openDialog() sends via

      var ckeditorAjaxDialog = Drupal.ajax({
        dialog: dialogSettings,
        dialogType: 'modal',
        selector: '.ckeditor-dialog-loading-link',
        url: url,
        progress: {'type': 'throbber'},
        submit: {
          editor_object: existingValues
        }
      });
      ckeditorAjaxDialog.execute();

The server receives this:

array (
  'editor_object' => 
  array (
    'src' => '',
    'alt' => '',
    'width' => '',
    'height' => '',
    'data-align' => 'none',
    'hasCaption' => 'false',
  ),
  'dialogOptions' => 
  array (
    'title' => 'Insert Image',
    'dialogClass' => 'editor-image-dialog editor-dialog',
    'autoResize' => 'true',
  ),
  '_drupal_ajax' => '1',
  'ajax_page_state' => 
  array (
    'theme' => 'seven',
    'theme_token' => 'Abv68lcUseVAp4Pb9wX9A-XQz8UsMfDBXmWPRfej0wM',
    'libraries' => 'ckeditor/drupal.ckeditor,ckeditor/drupal.ckeditor.plugins.drupalimagecaption,classy/base,comment/drupal.comment,contextual/drupal.contextual-links,contextual/drupal.contextual-toolbar,core/drupal.active-link,core/drupal.autocomplete,core/drupal.collapse,core/drupal.date,core/drupal.dropbutton,core/drupal.states,core/html5shiv,core/jquery.form,core/normalize,file/drupal.file,filter/drupal.filter,image/form,menu_ui/drupal.menu_ui,node/drupal.node,path/drupal.path,seven/global-styling,seven/node-form,shortcut/drupal.shortcut,text/drupal.text,toolbar/toolbar,toolbar/toolbar.escapeAdmin,tour/tour,user/drupal.user.icons',
  ),
)

EditorImageDialog only cares about the editor_object part there, it does this:

    if (!$image_element = $form_state->get('image_element')) {
      $user_input = $form_state->getUserInput();
      $image_element = isset($user_input['editor_object']) ? $user_input['editor_object'] : [];
      $form_state->set('image_element', $image_element);
    }

Note that it wants to remember the data it got from the editor via Form State.

Step 2: upload image

New form state is built. The input now originates from the form, and because a new FormState is built, we don't have image_element anymore:

array (
  'fid' => 
  array (
    'fids' => '',
  ),
  'attributes' => 
  array (
    'alt' => '',
    'width' => '',
    'height' => '',
    'data-align' => 'none',
  ),
  'form_build_id' => 'form-ASnrEiuOURTgdB3Lkv-WcHb-V0ad3NDDryXqOJnwMYc',
  'form_token' => 'jTzMGooEQDr50Qdhn9xwxMkzGTrYM3RNkqWVlZLCwl0',
  'form_id' => 'editor_image_dialog',
  '_triggering_element_name' => 'fid_upload_button',
  '_triggering_element_value' => 'Upload',
  '_drupal_ajax' => '1',
  'ajax_page_state' => 
  array (
    'theme' => 'bartik',
    'theme_token' => 'Abv68lcUseVAp4Pb9wX9A-XQz8UsMfDBXmWPRfej0wM',
    'libraries' => 'ckeditor/drupal.ckeditor,ckeditor/drupal.ckeditor.plugins.drupalimagecaption,classy/base,comment/drupal.comment,contextual/drupal.contextual-links,contextual/drupal.contextual-toolbar,core/drupal.active-link,core/drupal.autocomplete,core/drupal.collapse,core/drupal.date,core/drupal.dialog.ajax,core/drupal.dropbutton,core/drupal.states,core/html5shiv,core/jquery.form,core/jquery.form,core/normalize,editor/drupal.editor.dialog,file/drupal.file,file/drupal.file,filter/drupal.filter,image/form,menu_ui/drupal.menu_ui,node/drupal.node,path/drupal.path,seven/global-styling,seven/node-form,shortcut/drupal.shortcut,text/drupal.text,toolbar/toolbar,toolbar/toolbar.escapeAdmin,tour/tour,user/drupal.user.icons',
  ),
)

Step 3: when saving

When we hit "Save", data analogous to that in the previous step is sent, i.e. again without editor_object. Form state is recreated again, and is thus again missing the original data we stored in $form_state->set('image_element', $image_element like we did in step 1.

Because of this, ::buildForm() omits the data-align and data-caption form items, and hence also their values, and hence also doesn't send them back in its response.

In other words: we literally show form fields to the user that only exist in the first step, when submitting the form, the form is rebuilt and then resubmitted, with the form system blissfully unaware that these form fields even exist, and hence their values are not sent back.


I know that this form does quite unconventional stuff, but that's because it needs to integrate with unconventional stuff. I'm hoping a form system maintainer can give some feedback on how to resolve this!

kattekrab’s picture

Title: EditorImageDialog alignment & captioning are not working » (regression) EditorImageDialog alignment & captioning are not working
DuaelFr’s picture

I just closed #2533738: Image align radio and caption checkbox in CKeditor does not work anymore as duplicate as this one has seen more work done. Someone not browsing from a mobile could copy the IS and beta evaluation from the mentioned issue and improve it using the content of #18.

kattekrab’s picture

Issue summary: View changes
kattekrab’s picture

Issue summary: View changes
larowlan’s picture

Assigned: larowlan » tim.plunkett

hoping @tim.plunkett might know answer to #18

larowlan’s picture

looks like @kattekrab already did the issue summary update

larowlan’s picture

Issue tags: -Needs beta evaluation

no beta eval needed for major bugs, we can always fix them :)

kattekrab’s picture

bartik captions component testing blocked by this issue - adding as related.
#2398459: Clean up "captions" component in Bartik

Wim Leers’s picture

@kattekrab This does not block that issue at all, see #2398459-26: Clean up "captions" component in Bartik.

kattekrab’s picture

@Wim Leers oh good! thanx :)

kattekrab’s picture

Issue summary: View changes

Updated issue summary with a work around - editing the source manually to include data-align and data-caption attributed in the img tag.

As suggested by @jibran at https://www.drupal.org/node/2546456#comment-10189750

kattekrab’s picture

Issue summary: View changes
tim.plunkett’s picture

I haven't made any progress on this, but I wanted to chime in to say I am actively working on it.

StuartJNCC’s picture

+1 tim.plunkett

Whilst I agree with larowlan in #9 that it doesn't meet the criteria for "critical", I also agree with #8 that this is such an "in your face" bug that it is going to be encountered almost immediately by anyone reviewing or trying out Drupal. Consequently, it ought to be a release blocker. Not sure how to tag it to express that formally!

DuaelFr’s picture

Issue tags: +Release blocker

I totally agree.
Tagging as Release blocker to keep everyone's attention on that issue :)

Wim Leers’s picture

Once Tim is able to pinpoint the root cause, I solemnly swear I'll write the integration test coverage that would've prevented this regression from being introduced in the first place! #feelingguilty

larowlan’s picture

Fwiw I still think 10 is valid

Wim Leers’s picture

#35: it's not valid, see #17.

kattekrab’s picture

Issue summary: View changes

- making @DuaelFr's git bisect investigations more prominent in the IS.

kattekrab’s picture

Issue summary: View changes

with the link to the commit this time

Wim Leers’s picture

effulgentsia’s picture

Priority: Major » Critical
Issue tags: -Release blocker +blocker

#2521820-22: Update CKEditor library to 4.5.3 says this blocks that issue, so promoting to Critical.

xjm’s picture

Thanks everyone for identifying this issue.

The bug, while indeed nasty, is not in itself critical.

Can we clarify how/why this blocks #2521820: Update CKEditor library to 4.5.3? I can understand the desire to fix it before committing that in order to maybe ensure the regression does not get compounded, or because the changes would conflict, but the worst-case scenario should still be that we update CKEditor without this being fixed, and create RC1 without this being fixed. Thanks!

tim.plunkett’s picture

I am also interested in the response to #41.

----

I tried a bunch of other fancier things, but every time I tried to distill the patch down to its simplest form, it kept coming back to the fact that $form_state->getUserInput()['attributes'] and $form_state->getUserInput()['editor_object'] had exactly the same values but at different times during the request.

Manual testing with this patch should show it works.
Note the use of #parents in the code (not seen in the patch) that set it to use 'attributes'.

Wim Leers’s picture

#41: Drupal 8 extends CKEditor's image2 plugin, and even in a layered manner:

  • core/modules/ckeditor/js/plugins/drupalimage/plugin.js
  • core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js

(This was implemented and designed in close collaboration with the CKEditor team.)

When we updated to CKEditor 4.4, we had to almost completely rewrite those plugins on top of image2 (before that they were written on top of image), see #2039163: Update CKEditor library to 4.4.

If we commit CKE 4.5.3 without being able to also thoroughly test Drupal 8's extensions to image2, then we risk breaking even the JS, making it even harder to fix this bug.

tim.plunkett’s picture

Given this flow:

  1. Open image dialog
  2. Select align right
  3. Upload image

Steps 2 and 3 could also be reversed.

Before the AJAX changes, $form_state was cached and would have $form_state->get('image_element') in it.
In certain cases, the stored image_element can get stale anyway, and the formBuilder->processForm() call in FileWidgetAjaxController happens to handle pulling the correct values from user input.

In HEAD, we don't have the extra processForm() call. But we do have 'attributes' in the user input, and this change lets the values automatically get mapped to their respective elements.

tim.plunkett’s picture

@Wim Leers pointed out that this fix is only coincidental, and does not work for the hasCaption portion. I'm not even going to try to figure out why it worked for some and not others.

Why can't we just mark this form as cached and restore the old functionality? I need to dust off some brain cells before determining if this is actually okay, or someone else can chime in.

$form_builder->buildForm() does two very important things that this test is missing:
It sets the requestMethod on FormState, and it populates the user input from the request.

However, I'm getting a "LogicException: The database connection is not serializable." when serializing the form, which has fileStorage, which has database (the connection).

The changes other than the setCached() in EditorImageDialog are from Wim, thanks!

The interdiff is from @larowlan's test

tim.plunkett’s picture

Alternately, we could do this, if wanting to continue to bypass buildForm()

The last submitted patch, 45: 2540850-editor-45.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 46: 2540850-editor-46.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.84 KB
840 bytes

Unlike the other entity handlers that extend from EntityHandlerBase, none of the implementations of EntityStorageSchemaInterface use a high level base class.
Many extend from SqlContentEntityStorageSchema, which stores a connection to the database. However, it does not use DependencySerializationTrait. \Drupal\file\FileStorageSchema was the object blowing up those tests.

larowlan’s picture

Issue tags: +Needs manual testing
+++ b/core/modules/editor/src/Form/EditorImageDialog.php
@@ -63,14 +63,22 @@ public function getFormId() {
+    $image_element = $form_state->get('image_element');

Should we wrap this in a !isset($image_element) and add $image_element = to the start of $form_state->set('image_element', $form_state->getUserInput()['editor_object']); to save the extra ->get call, or is that micro-optimizing/deck chair shuffling?

We need a manual tester here now.

kattekrab’s picture

did someone say manual testing? On it.

kattekrab’s picture

Issue summary: View changes
FileSize
481.79 KB

Hooray! It works!

Also tested using quick edit, and editing the image once saved.

Thanks everyone!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: -Needs manual testing
FileSize
6.72 KB
1.19 KB

Deck chairs shuffled!
Thanks @kattekrab for manually testing.
Wim and I tested manually as well.

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
57.03 KB
54.44 KB

I also tested manually and as the testbot is happy and the patch respects coding standards and includes a extensive documentation in comments, I have the immense pleasure to mark this as RTBC.
Thanks to everybody for that fix!

Before :'(

After \o/

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, SO happy this got fixed in time for RC1!!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 3db6fb5 on 8.0.x
    Issue #2540850 by tim.plunkett, larowlan, kattekrab, DuaelFr, emma.maria...
kattekrab’s picture

Yaaaaaay!!!!!!!

Hi-5s all round!

This bug made me feel really stupid when I first hit it - like I must have been doing something wrong. So it's really great to see it fixed.

Thanks so much everyone :-)

Status: Fixed » Closed (fixed)

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