Problem/Motivation

Steps to reproduce:

  1. Install D8 HEAD (http://drupalcode.org/project/drupal.git/commitdiff/396cacba7cbebb380e78... or later).
  2. Create an article node with a non-empty body.
  3. Go to /node/1.
  4. Click the contextual links, notice that "Quick edit" is missing.
  5. Look at the browser console, notice the JS errors.
  6. Look at the /quickedit/attachments XHR, notice that it's printing a PHP fatal!

This was introduced by #2030605: Expand Editor with methods, but stayed under the radar because it only happens when you have a page that uses editor.module's in-place editor. For example: this bug does not happen on the frontpage of a default install, but it does on full node pages.
That being said, it should've been caught by a test.

Proposed resolution

Fix it + write regression test.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#1 editor_breaks_quickedit-2263351-1-test_only.patch1.38 KBwim leers
#1 editor_breaks_quickedit-2263351-1.patch2.69 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new editor_breaks_quickedit-2263351-1.patch2.69 KB
new editor_breaks_quickedit-2263351-1-test_only.patch1.38 KB

The "test only" patch should fail.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I was wondering what that didn't work (anymore) :)

As did @EclipseGc in IRC...

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php
@@ -78,10 +78,11 @@ public function getAttachments() {
     $formats = array();
     foreach ($user_format_ids as $format_id) {
-      $editor = editor_load($format_id);
-      $editor_id = $editor->getEditor();
-      if ($editor && isset($definitions[$editor_id]) && isset($definitions[$editor_id]['supports_inline_editing']) && $definitions[$editor_id]['supports_inline_editing'] === TRUE) {
-        $formats[] = $format_id;
+      if ($editor = editor_load($format_id)) {

Not specific to this fix, but wondering why this doesn't load all editors together and foreaches on that, then it simply won't do anything if there's nothing?

There is no editor_load_multiple(), but it can use the entity manager or entity_load_multiple().

#2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities would help, which currently doesn't deprecate/replace entity_load() (please review that :))

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -Quickfix +Quick fix

Just learned a new thing

eclipsegc’s picture

eclipsegc commented
Status: Needs review » Reviewed & tested by the community

This looks pretty rtbc to me, and fixes a huge wtf imo.

Eclipse

Bojhan’s picture

Bojhan commented

Hah, I did notice this - thought it was my computer acting up.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 5f79a75 on 8.x by webchick: 
    Issue #2263351 by Wim Leers: Text Editor's in-place editing plugin broke...
jcisio’s picture

jcisio commented

Notice: the default.settings.php change should have been part of #2241633: Simplify site-specific service overrides.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#8: Wrong issue?

jcisio’s picture

jcisio commented

Nope, it was a notice for a mistake in webchick's commit http://drupalcode.org/project/drupal.git/commitdiff/5f79a75. And it's this issue ;)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hm. Strange. Re-appled that hunk of the diff and committed and pushed to 8.x. Thanks!

  • Commit ed8dfae on 8.x by webchick: 
    Issue #2263351 by jcisio: Replace accidentally removed part of default....
jcisio’s picture

jcisio commented

I didn't mean a patch, just a note for whom who do git blame, because I thought that extra note had already been removed in #2241633: Simplify site-specific service overrides. But in fact no. I've just checked again in that issue, catch reverted the commit, but then undid that revert.

So this commit is actually good. Thanks.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

Status: Fixed » Closed (fixed)

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