EditorPluginInterface should extend PluginFormInterface

I am only able to find minor issue.

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/StylesCombo.php
@@ -65,7 +65,7 @@ public function getButtons() {
+   * Implements \Drupal\ckeditor\Plugin\CKEditorPluginConfigurableInterface::buildConfigurationForm().

@@ -93,7 +93,7 @@ public function settingsForm(array $form, FormStateInterface $form_state, Editor
+   * #element_validate handler for the "styles" element in buildConfigurationForm().

+++ b/core/modules/ckeditor/tests/modules/src/Plugin/CKEditorPlugin/LlamaContextualAndButton.php
@@ -60,7 +60,7 @@ function getFile() {
+   * Implements \Drupal\ckeditor\Plugin\CKEditorPluginConfigurableInterface::buildConfigurationForm().

Unrelated. Revert this please.

What about all the documentation we are removing form the EditorPluginInterface and ImageToolkitInterface?

I'd suggest we should ping maintainers of these two component for this change.

PS: I mean Editor and Image maintainers not Plugin maintainer :D

PluginFormInterface was added August 1, 2013. EditorPluginInterface was added January 16, 2013.

That's why.

#5: thanks :)

1. +++ b/core/modules/editor/src/Plugin/EditorPluginInterface.php
   @@ -34,53 +34,6 @@
   -   * If the editor's behavior depends on extensive options and/or external data,
   -   * then the implementing module can choose to provide a separate, global
   -   * configuration page rather than per-text-format settings. In that case, this
   -   * form should provide a link to the separate settings page.
   

   This is lost.

2. +++ b/core/modules/editor/src/Plugin/EditorPluginInterface.php
   @@ -34,53 +34,6 @@
   -   * The contents of the editor settings are located in
   -   * $form_state->getValue(array('editor', 'settings')). Calls to form_error()
   -   * should reflect this location in the settings form.
   

   This is also lost.

   And this also introduces a subtle but important conflict. Everything used to be consistently named "settings": in the code, in the interface (getDefaultSettings()) and in the config entity. This patch removes that consistency. That's acceptable, but it's definitely a subtle regression.

   I think we should consider renaming everything from "settings" to "configuration" for consistency.

3. +++ b/core/modules/editor/src/Plugin/EditorPluginInterface.php
   @@ -34,53 +34,6 @@
   -   * Values in $form_state->getValue(array('editor', 'settings')) are saved by
   -   * Editor module in editor_form_filter_admin_format_submit().
   

   Also lost.

Added the lost comments (see: #7).
Reworked everything to configuration instead of settings to keep the consistency.
Added type-hinting to the plugin generated by the plugin manager in Editor.php (see line 85 - 86).

Tests are failing because \Drupal\Tests\editor\Unit\EditorConfigEntityUnitTest hasn't been updated yet. Could you please also update that unit test?

+++ b/core/modules/editor/src/Entity/Editor.php
@@ -82,7 +83,8 @@ public function __construct(array $values, $entity_type) {
     // Initialize settings, merging module-provided defaults.
-    $default_settings = $plugin->getDefaultSettings();
+    /* @var EditorPluginInterface $plugin */
+    $default_settings = $plugin->getDefaultConfiguration();
     $default_settings += \Drupal::moduleHandler()->invokeAll('editor_default_settings', array($this->editor));
     \Drupal::moduleHandler()->alter('editor_default_settings', $default_settings, $this->editor);
     $this->settings += $default_settings;

s/settings/configuration/ several times here.

This would also cause a hook name to be changed, hence we'll have to update its documentation in editor.api.php as well.

I have implement all changes which are mention (editor-2326721-7.patch)

The patch doesn't apply to the latest version of Drupal 8. Apply it to your local version, or if you're using a branch, switch to that. Then, you can do git pull --rebase to bring it to the latest version. Then you can just recreate the patch file and upload it :)

tags can be confusing.

Thank you for working on this issue.

Note, tags should be separated with a comma, not a space.

The Queue tag was used to line up issues before the sprint. (#2407325: preparing for a sprint, would be good to have a list of candidate issues)
When you work on an issue please add: SprintWeekend2015 (note, no @ or # at the beginning) according to that and https://groups.drupal.org/node/447258

@Wim Leers
Thanks will update and recreate the patch

create patch with latest code

re verifying and uploading new patch

The fatal:
PHP Fatal error: Unsupported operand types in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/editor/src/Entity/Editor.php on line 84

- Test was using wrong method.
- Few indentation correction.
- Commented codes removed.

Any idea why it is failing with unsupported operand types error. According to the patch in #40 the function getDefaultSettings() has "only" been renamed to getDefaultConfiguration() (or is there something more in it?). Anyone please help!

$this->settings and $plugin->getDefaultConfiguration() both are of array types too.

Uploading new patch with some minor fixes. I am still waiting for some help #42

Would love to help with #42, but the patch doesn't apply anymore. If you can get another working test run, I'll try to help :)

Trying.

@tim.plunkett thank you for review this :)
Here the patch.

Great! Now let's make the test green :)

The cause of the fatal:

PHP Fatal error:  Unsupported operand types in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/editor/src/Entity/Editor.php on line 85

+++ b/core/modules/editor/src/Entity/Editor.php
@@ -80,7 +80,9 @@ public function __construct(array $values, $entity_type) {
+    $this->settings += $plugin->getDefaultConfiguration();

So either $this->settings is not an array, or whatever $plugin->getDefaultConfiguration() is returning is not an array.

Let me see something first.

There is a error in core/modules/editor/tests/src/Unit/EditorConfigEntityUnitTest::testCalculateDependencies()

line 115 when:

$entity = new Editor($values, $this->entityTypeId);

Because in core/modules/editor/src/Entity/Editor::__construct(), $plugin->getDefaultConfiguration() isn't a array, seem it is NULL.

Because in core/modules/editor/src/Entity/Editor::__construct(), $plugin->getDefaultConfiguration() isn't a array, seem it is NULL.

Isn't it because of the method rename?

getDefaultConfiguration() called more than once...

@tim.plunkett because the patch fail due $plugin->getDefaultConfiguration() sometime is NULL.

Look this tests:

Tip:
The error more concise: 2326721-65-2-do-not-test.patch

Ups sorry is test-only.

Umm my mistake :)

#64 suggest.

Already #69 patch resolve that.

This is a nice improvement :) I only found some minor naming and documentation issues:

1. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -248,7 +248,7 @@ public function settingsFormSubmit(array $form, FormStateInterface $form_state)
   +  public function getJSConfiguration(EditorEntity $editor) {
   

   If we are renaming this method anyway, we can fix the capitalization error at the same time: JS > Js.

2. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -369,7 +369,7 @@ public function getLibraries(EditorEntity $editor) {
   +   * @see getJSConfiguration()
   

   I don't know if it's required, but I would add a bit more context to this, as the documentation in this form refers to a global function. Something like @see self::getJSConfiguration() would work.

3. +++ b/core/modules/ckeditor/src/Tests/CKEditorTest.php
   @@ -71,7 +71,7 @@ protected function setUp() {
   +   * Tests CKEditor::getJSConfiguration().
   

   Missing namespace.

Applied the minor changes.

Thanks! Could you post an interdiff as well?

Done!
I couldn't understand your comment about the missing namespace tho.

Thanks for the interdiff. It looks good!

When we refer to classes or interfaces in code comments, the Drupal coding standards require that we include the namespace as well, so we must use @param \Drupal\ckeditor\Plugin\Editor\CKEditor instead of just CKEditor, for example. Other coding standards sometimes allow developers to leave out the namespace, but we don't.

The interdiff doesn't have the namespacing but the patch file does.

This issue is a task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

+++ b/core/modules/ckeditor/src/Tests/CKEditorTest.php
@@ -71,7 +71,7 @@ protected function setUp() {
   /**
-   * Tests CKEditor::getJSSettings().
+   * Tests CKEditor::getJsConfiguration().
    */
   function testGetJSSettings() {

Lets update the test method name too.

Re-roll.

I'll post another patch that cleans up a few more things.

There's more we can improve, but for now I only improved the lines that were touched by the previous patch anyway.

Thanks!

Back to RTBC.

Thanks for the patch and for keeping this issue going.

Thanks also for adding the beta evaluation. Unfortunately though, it is not correct -- this is definitely not unfrozen. Unfrozen changes are only:

• CSS
• markup
• translatable strings
• documentation
• automated tests (excluding BrowserTestBase tests)

Additionally, the provision of a Drupal 6/7 to Drupal 8 migration path is not strictly coupled to the release cycle, so that code remains unfrozen.

In general, on its own, "replacing legacy code with best practices" is not an allowed change during the Drupal 8 beta. Review https://www.drupal.org/core/beta-changes. This patch does also introduce BC breaks that could impact existing implementations. I checked with @Wim Leers and he suggested that it also wouldn't really make sense to try to do this in a non-BC-breaking way in a minor release. So, I am postponing this issue to 9.x. Thanks everyone!

(Removing the unfrozen line from the summary to avoid future confusion and adding reviewer credit.)

I think we should consider a bc-compatible patch here for a late release of 8.x - while it wouldn't make things clearer, it might allow 9.x compatible cod to be written in 8.x

Let's see if we can such a BC-compatible patch done then.

Not sure if the Novice and the php-novice tags are still valid for this issue, can we remove them?

Good point!

We cannot do this in a BC-compatible way. Only in a half-BC-compatible way: we could change \Drupal\editor\Plugin\EditorPluginInterface to let it extend PluginFormInterface, but then all implementations of that interface will cease to work. We can "automatically fix" any implementations that extend EditorBase by letting EditorBase implement the PluginFormInterface methods, but then that'd mean letting that base class redirect all the "proper" method implementations to the "bad" (current) ones. Otherwise it's still a BC break. But then we're effectively encouraging incorrect implementations!

So, AFAICT there's no way to make this change in a BC-compatible way. In other words, I think this can only happen in D9.

We can "automatically fix" any implementations that extend EditorBase by letting EditorBase implement the PluginFormInterface methods, but then that'd mean letting that base class redirect all the "proper" method implementations to the "bad" (current) ones. Otherwise it's still a BC break. But then we're effectively encouraging incorrect implementations!

This would allow modules to update to the new interface though right? In which case I think it's worthwhile in 8.x, albeit ugly. If 9.x only drops backwards compatibility with 8.x, then we need to get our heads around problems like this.

Like this?

Note that the lack of changes to EditorBase subclasses proves that this maintains BC.

TODO: look into whether https://www.drupal.org/node/2774077 affects this or not.

That looks right to me.

Then I think this is ready to be committed.

So we should ideally @trigger_error('...', E_USER_DEPRECATED) in the bc methods on the base class, that will encourage people to implement the interface properly. As well as adding test coverage (i.e. a test subclass that doesn't have those methods) so we can confirm the bc layer works per #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases.

Or is that a separate issue? Hmm.

I wasn't sure about that either. SubFormState is so very new. This used to work just fine. So I think adopting SubFormState should be a separate issue. This issue is already more than 100 comments long. Adopting SubFormState should be less tricky and controversial, because it is solely internal refactoring.

#104: done.

CR created: https://www.drupal.org/node/2819753.

This implementation of buildConfigurationForm() should declare $editor as an optional parameter. It can't pull it from $form_state->get(), because nothing ever set it.

If this didn't work, then we'd have several failing tests for the CKEditor test coverage.

It's \editor_form_filter_admin_format_editor_configure() that sets it.

Excellent catch, that was indeed a small oversight on my part :)

Thanks for the reroll & explanation!

Committed aed10c0 and pushed to 8.3.x. Thanks!

Nice to rescue this from 9.x purgatory.

Cool :) AFAIK this is the first patch of its kind! Hopefully it's a solid precedent.

Publishing CR & unpostponing follow-up.