Overview of issue:
Disable the ckeditor from a vanilla install of drupal 8, then try to add a new piece of article content you will get the following error message:

"Error message
Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("The plugin (ckeditor) did not specify an instance class.") in "core/themes/seven/templates/page.html.twig" at line 103. in Twig_Template->displayWithErrorHandling() (line 279 of /home/quickstart/websites/aiyps.com/core/vendor/twig/twig/lib/Twig/Template.php).
The website has encountered an error. Please try again later. "

Expected behaviour
Was not expecting the error message, was expecting to be able to create article content.

Replicate
Install drupal 8
Goto Menu > Extend
de-select CKeditor
click "save configuration"

goto Menu > Content > Add Content
Select Article

You will receive error message.

Drupal 8 package details
Drupal 8 packaged version: 8.0-alpha1+186-dev
Last updated: May 29, 2013 - 12:45

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

floydm’s picture

I can confirm this error happens in the current build in git.

A bit more sleuthing and I figured out the editor gets set in and loaded from the following config files:

editor.editor.basic_html.yml
editor.editor.full_html.yml

Remove or rename those files and clear cache after disabling CKEditor and everything operates as expected.

larowlan’s picture

Issue tags: +Needs tests

Tagging

larowlan’s picture

Title: TWIG Error Runtime - when disabling ckeditor from modules list » When ckeditor is disabled, editor config entities that refer to it cause a Twig Exception
Status: Active » Needs review
FileSize
1.86 KB
1.3 KB

Fail and pass

larowlan’s picture

Component: theme system » editor.module

Correct module

quicksketch’s picture

Status: Needs review » Needs work

This patch deletes the configuration data of CKEditor upon disabling the module. This isn't standard practice in Drupal (yet, see #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed), where the expected behavior is that the user can turn back on a disabled module and the configuration will be restored as it was before the module was disabled.

Instead of deleting all configuration as part of hook_disable(), the typical approach to this problem is to check if the requirements are met before attempting to load the editor. When an editor entity is attempted to be loaded, we should check that the corresponding editor plugin still exists. If it doesn't, we should return FALSE, as if the editor did not exist at all.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.74 KB
3.01 KB

So this version doesn't delete the entities, it just catches the PluginException in the storage controller and doesn't load the editor entity. Lets see what this breaks.

larowlan’s picture

Green!

tim.plunkett’s picture

FileSize
1.87 KB
3.42 KB

I'd try to minimize the overridden part by calling the parent when possible.

Wim Leers’s picture

Title: When ckeditor is disabled, editor config entities that refer to it cause a Twig Exception » When CKEditor is disabled, editor config entities that refer to it cause a Twig Exception
Issue tags: +CKEditor in core

Great catch :) Will try to review later today. I dislike that we introduce a custom storage controller, but I guess that's the only way?

Wim Leers’s picture

#8: ckeditor-2007248-8.patch queued for re-testing.

Wim Leers’s picture

Title: When CKEditor is disabled, editor config entities that refer to it cause a Twig Exception » When CKEditor is disabled, editor config entities that refer to it cause a Twig exception
Status: Needs review » Needs work
Issue tags: +sprint, +Spark

Isn't this really just user error? It's the same as disabling a module that provides a new filter without removing that filter from all text formats where it was in use. That will also break.
So: are we really solving the right problem here?


In any case, this patch doesn't cover all use cases, the Twig exception still occurs when accessing admin/config/content/formats (using otherwise the same scenario as in the issue summary).

AFAICT the reason is editor_load(), which calls entity_load_multiple('editor') (i.e. loads all 'editor' entities), which in turn causes the override to have no effect, because it'll fall back to the parent implementation:

    if ($ids === NULL) {
      $result = parent::buildQuery($ids, $revision_id);
    }
Wim Leers’s picture

Issue tags: -sprint

.

larowlan’s picture

@WimLeers well my preference is to just plain delete it, as per patch at #3 but @quicksketch asked for something less aggressive.

Wim Leers’s picture

Status: Needs work » Postponed

#13: Good point.

#5 no longer holds, since consensus has been reached at #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed: the "disabled" module state will be removed.

Which means the patch in #3 is good to go, except that it should use hook_uninstall() rather than hook_disable(), because the latter is being removed in #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed. Postponing on that issue getting committed, then we should reroll the patch in #3.

David_Rothstein’s picture

Status: Postponed » Needs work

The method in #3 relies on every implementing module (CKEditor in this case, but imagine TinyMCE as another) dealing with this on disable and/or uninstall (note that for disable, I would think disabling the plugin a la https://drupal.org/node/1926376 may be a better way to go). So shouldn't it be dealt with at least one level up (in the Editor module) if not higher?

And there are other ways besides disable/uninstall that a plugin can disappear. For example, a module which defined several editor plugins and then removed one (think WYSIWYG module dropping support for a particular editor) would have to do the same thing on update...

And in the general case, is the approach in #3 even possible? Does the implementing module always know all the places that use it?

So overall seems like this issue might be a duplicate of #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies)...

Wim Leers’s picture

Status: Needs work » Closed (duplicate)

Thanks for your thoughtful feedback, David_Rothstein — much appreciated!

I agree with your overall assessment. So: marking this issue as a duplicate of #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies).

larowlan’s picture

Issue summary: View changes
Status: Closed (duplicate) » Postponed

I think this should be postponed on the meta, not necessarily closed until such time as there is a resolution there.
Thoughts?

Wim Leers’s picture

I guess you're right.

I assumed this was going to be fixed by #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies) but you're implying that we'll still need to do some work here to leverage the work done in #1881630 (once it lands) — which is probably right.

swentel’s picture

Talked this through with alexpott on IRC. #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall is getting into shape which allows us to set dependencies in config entities and are automatically discovered on uninstall (and also deleted). However, to keep the size and complexity of the other issue relatively small, we'll follow up on this when that one is committed and decide then whether to remove this on hook_uninstall() or use the soft dependency api.

Wim Leers’s picture

Title: When CKEditor is disabled, editor config entities that refer to it cause a Twig exception » When CKEditor module is uninstalled, the right text editor config entities are affected but their labels are missing
FileSize
102.71 KB

#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall got committed. That fixed this problem! :)

However, in testing it, I found a minor bug: text editor entity labels are missing.

So let's repurpose this issue to fix that.

Wim Leers’s picture

Priority: Normal » Minor
Status: Postponed » Active
Wim Leers’s picture

Status: Active » Needs review
Issue tags: +sprint, +Novice
FileSize
756 bytes

The cause was simple: Text Editor config entities themselves don't have labels; their associated Filter Format config entity does!

Wim Leers’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Makes total sense.

xjm’s picture

FileSize
597 bytes

Rerolled for PSR-4.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6352edf and pushed to 8.x. Thanks!

  • Commit 6352edf on 8.x by alexpott:
    Issue #2007248 by larowlan, tim.plunkett, xjm, Wim Leers: Fixed When...
Wim Leers’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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