Expand Editor with methods

I tried to do this, but failed, because it's not at all clear what needs to happen. Postponed on feedback at #2016679-13: Expand Entity Type interfaces to provide methods, protect the properties.

I did this because of #2016679-14: Expand Entity Type interfaces to provide methods, protect the properties and #15. But I still don't get *why*. See #16 there.

After a brief chat with Berdir, it is clear that I missed the key point in the #2 patch. Fixing that.

Editor->editor and Editor->settings should be protected. Editor->format can't be protected yet because it's an entity key.

.

Since #2016679-29: Expand Entity Type interfaces to provide methods, protect the properties, I finally fully grok the what and why. I'll make this happen.

3: expand_editor_methods-2030605-3.patch queued for re-testing.

I have tried to do a review. But the patch failed me.
There are a lot of changes to this module since jan 9th.
For instance in the file CKEditorPluginManager.php there is a change made to the function getEnabledPlugins, only that function no longer exists.

error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginInterface.php:79
error: core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginInterface.php: patch does not apply
error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.php:63
error: core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.php: patch does not apply
error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php:55
error: core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php: patch does not apply
error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.php:82
error: core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.php: patch does not apply
error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorAdminTest.php:142
error: core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorAdminTest.php: patch does not apply
error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorLoadingTest.php:121
error: core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorLoadingTest.php: patch does not apply
error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorPluginManagerTest.php:103
error: core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorPluginManagerTest.php: patch does not apply
error: patch failed: core/modules/ckeditor/tests/modules/lib/Drupal/ckeditor_test/Plugin/CKEditorPlugin/LlamaContextual.php:28
error: core/modules/ckeditor/tests/modules/lib/Drupal/ckeditor_test/Plugin/CKEditorPlugin/LlamaContextual.php: patch does not apply
error: patch failed: core/modules/ckeditor/tests/modules/lib/Drupal/ckeditor_test/Plugin/CKEditorPlugin/LlamaContextualAndButton.php:31
error: core/modules/ckeditor/tests/modules/lib/Drupal/ckeditor_test/Plugin/CKEditorPlugin/LlamaContextualAndButton.php: patch does not apply
error: patch failed: core/modules/editor/editor.module:164
error: core/modules/editor/editor.module: patch does not apply
error: patch failed: core/modules/editor/lib/Drupal/editor/EditorInterface.php:2
error: core/modules/editor/lib/Drupal/editor/EditorInterface.php: patch does not apply
error: core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php: No such file or directory
error: patch failed: core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php:37
error: core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php: patch does not apply
error: patch failed: core/modules/editor/tests/modules/lib/Drupal/editor_test/Plugin/Editor/UnicornEditor.php:42
error: core/modules/editor/tests/modules/lib/Drupal/editor_test/Plugin/Editor/UnicornEditor.php: patch does not apply].

No need to post the entire list of failures. Just saying "the patch doesn't apply anymore" is enough :) ;)

If you want to work on this, I'll review your patches!

Hurray — thanks, daffie! :)

Working on it as part of core mentoring hours

Yay :) I'll provide reviews!

Ok sorry, misunderstood that one, let's try again.

Assigning to me for review (DDD Szeged sprint).

• Patch no longer applied, so I re-rolled it.
• Editor has a public $format property, for which no getter/setter is added in this patch. I wanted to convert it to a protected property, but this causes the ConfigStorageController to fail. Ok to leave public without adding a getter (since I assume getFilterFormat() should be used)? I did add a @see directive pointing to getFilterFormat().
• DrupalImageCaption::isEnabled() and CKEditor tests were still using $editor->settings instead of using the getSettings().
• Comment in CKEditorPluginContextualInterface::isEnabled was still refering to the property instead of recommend getSettings().
• editor_form_filter_admin_format_submit() still uses the $settings-property directly instead of using the &getSettings() getter. Is that acceptable, or should all uses of the property be replaced?
• After discussion with Wim Leers, added getter/setting for the $image_upload.
• Changed some of the comment summaries to use third-person

Wim Leers & I looked into 2 exceptions that occur during Editor Admin Testing. However, both exceptions also occur on a clean 8.x HEAD (confirmed on two separate environments). Setting to needs review to see how the testbot reacts.

I hadn't actually seen the patch mr.baileys worked on, we just discussed a few things (as he mentioned). Patch looks good to me. The second bullet in #19 needs feedback from e.g. Tim Plunkett, whom I just pinged.

Thanks! :)

#19.2 is a good question, and one that had me confused at first.

/**
 * Defines the configured text editor entity.
 *
 * @ConfigEntityType(
 *   id = "editor",
 *   label = @Translation("Text Editor"),
 *   entity_keys = {
 *     "id" = "format"
 *   }
 * )
 */
class Editor extends ConfigEntityBase implements EditorInterface {

Because it's declared as the ID key, $editor->format is the same as $editor->id(). I need to file an issue about ConfigStorageController assuming that's public, but for now you should leave it public. One other thing to consider (so that everyone stops having the same confusion) is to rename $editor->format to $editor->id, like 90% of the other entity types.

I didn't review the patch yet, but just use ->id() where appropriate, and I'll crosslink the ConfigStorageController issue when I create it.

1. +++ b/core/modules/ckeditor/ckeditor.admin.inc
   @@ -39,7 +39,8 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
   +  $settings = $editor->getSettings();
   +  foreach ($settings['toolbar']['rows'] as $row_number => $row) {
   
   +++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginInterface.php
   @@ -79,11 +79,15 @@ public function getFile();
   +   * $settings = $editor->getSettings();
   +   * $plugin_specific_settings = $settings['plugins'][$plugin_id];
   

   This makes me wonder if a function getSetting($key) method would be a nice addition.

2. +++ b/core/modules/editor/lib/Drupal/editor/EditorInterface.php
   @@ -21,4 +21,51 @@
   +   * @see setSettings()
   ...
   +   * @see getSettings()
   ...
   +   * @see setImageUpload()
   ...
   +   * @see getImageUpload()
   

   I think these are redundant

3. +++ b/core/modules/editor/lib/Drupal/editor/EditorInterface.php
   @@ -21,4 +21,51 @@
   +   * @param array
   ...
   +   * @param array
   

   These should repeat the variable name

4. +++ b/core/modules/editor/lib/Drupal/editor/EditorInterface.php
   @@ -21,4 +21,51 @@
   +  public function setSettings(array $settings);
   ...
   +  public function setImageUpload(array $image_upload);
   
   +++ b/core/modules/editor/lib/Drupal/editor/Entity/Editor.php
   @@ -123,4 +125,38 @@ protected function editorPluginManager() {
   +  public function setSettings(array $settings) {
   +    $this->settings = $settings;
   +  }
   ...
   +  public function setImageUpload(array $image_upload_settings) {
   +    $this->image_upload = $image_upload_settings;
   +  }
   

   Generally we make our setters fluent, meaning they should return $this so they can be chained. The docblock should say @return $this, no description needed on the next line.

5. +++ b/core/modules/editor/lib/Drupal/editor/EditorInterface.php
   @@ -21,4 +21,51 @@
   +  public function getImageUpload();
   

   I think this (and the setter) should be getImageUploadSettings(). I would expect a method called getImageUpload to return a file or something

6. +++ b/core/modules/editor/lib/Drupal/editor/Form/EditorImageDialog.php
   @@ -62,7 +63,7 @@ public function buildForm(array $form, array &$form_state, FilterFormat $filter_
   +      '#upload_location' => $image_upload['scheme'] . '://' . $image_upload['directory'],
   

   Out-of-scope follow-up: It'd be nice to have a helper method for this

Thanks for the review!

1. This function returns a nested array, wouldn't adding a getSetting($key) lead to awkward invocations like $this->getSettings('toolbar')['rows']?
2-5. Fixed
6. leaving for a follow-up.

#2224833: Remove redundant ID manipulation in ConfigEntityStorage::save() removed the need for a public ID property

1. Fixed a bunch of docs nitpicks.
2. #2220757: Limit length of Config::$id to something <= 166 characters broke #2224833: Remove redundant ID manipulation in ConfigEntityStorage::save(), requiring once again a public ID property. Fix included now.
3. We hadn't actually made the properties protected yet, so did that too — but apparently you have to override EntityInterface::toArray() then. Left a comment at #2016679-54: Expand Entity Type interfaces to provide methods, protect the properties regarding that.

IMO this is now good to go.

Looks great, thanks Wim!

Quick reroll, minor conflict with #2225955: Improve the DX of writing entity storage classes.

#29: thanks for the reroll! :)

Hrm. Not sure why the patch was marked as failed testing, since it is showing up green and there are no failures or exceptions in the test log, so back to RTBC.

29: editor-2030605-29.patch queued for re-testing.

Looks consistent with other patches doing a similar thing.

Committed and pushed to 8.x. Thanks!