Problem/Motivation

Part of #2959269: [meta] Core should not trigger deprecated code except in tests and during updates

This issue is related to deprecation errors triggered by Drupal\editor\Plugin\EditorBase related to this change record: https://www.drupal.org/node/2819753 from this issue: #2326721: EditorPluginInterface should extend PluginFormInterface

EditorBase::settingsForm(), ::settingsFormValidate() and ::settingsFormSubmit() are all deprecated in favor of the other settings-related methods on EditorBase. These methods call @trigger_error().

It's unlikely that a subclass which uses these methods will call the parent method, so the errors will not be triggered.

Further complicating matters, when an editor settings form is displayed, the controller looks for either settingsFormValidate() (deprecated) or validateConfigurationForm() (not deprecated), and calls settingsFormValidate() if it's present. This means any form which subclasses EditorBase but does not implement validateConfigurationForm() will always trigger the deprecation error.

In addition, these deprecated methods are marked as {@inheritdoc}, but they don't belong to an existing interface. So their deprecation messages are not present, and don't offer a way to fix the error.

Proposed resolution

Figure it out. :-)

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 3002933-11.patch9.25 KBalexpott
#11 10-11-interdiff.txt589 bytesalexpott
#10 3002933-10.patch9.66 KBalexpott
#10 5-10-interdiff.txt5.42 KBalexpott
#5 3002933-5.patch9.2 KBalexpott
#5 2-5-pseudo-interdiff.txt4.94 KBalexpott
#2 3002933_2.patch7.57 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Active » Needs review
FileSize
3002933_2.patch7.57 KB

Super explodey patch.

You can reduce the blast from CKEditorAdminTest by implementing a no-op validateConfigurationForm(), but even then, it still implements settingsForm() and settingsFormSubmit() even though those are supposed to be deprecated.

Should EditorBase be deprecated? And if so, in favor of what?

Status: Needs review » Needs work

The last submitted patch, 2: 3002933_2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
2-5-pseudo-interdiff.txt4.94 KB
3002933-5.patch9.2 KB

Here's what I think @Wim Leers intended... going to ping him this issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Also I think the CR is not quite right. https://www.drupal.org/node/2819753 says:

any Text Editor plugin that extends from that base class won't need to be changed at all.

Whereas what I think needs to happen is that the plugin should be updated to implement the \Drupal\Core\Plugin\PluginFormInterface methods.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Contrib modules to look at are (from http://grep.xnddx.ru/search?text=EditorBase&filename= ):

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Needs no changes

Needs changes

So interestingly the only one that's not triggering deprecations atm is ace_editor because it overrides all 3 methods :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +@deprecated
Related issues: +#2326721: EditorPluginInterface should extend PluginFormInterface

Here's what I think @Wim Leers intended... going to ping him this issue.

Yes and no 🙂

#2326721: EditorPluginInterface should extend PluginFormInterface was probably one of the earliest deprecations. And in #2326721-99: EditorPluginInterface should extend PluginFormInterface I wrote:

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

Changing subclasses is exactly what #5 is doing. Hence "no, #5's interdiff is not what I intended".

But … since then we've gained a lot more experience on how to deal with deprecations. And that's where the "yes" in "yes and no" comes from.

My only concern is that this means we will have no test coverage proving the old mechanism still works but triggers a deprecation error. \Drupal\Tests\editor\Unit\EditorBaseTest doesn't quite do that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue tags: +Needs change record updates
FileSize
5-10-interdiff.txt5.42 KB
3002933-10.patch9.66 KB

Ah I see yeah the code looks like it has half dealt with the deprecation and is triggering it from the wrong place. The patch attached completes the deprecation and properly tests it. We should also update the CR when this is committed to reflect reality.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
10-11-interdiff.txt589 bytes
3002933-11.patch9.25 KB

And now we can get rid of a pointless change too...

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

This looks perfect :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/editor/tests/src/Unit/EditorBaseTest.php
@@ -18,6 +20,10 @@ class EditorBaseTest extends UnitTestCase {
+   *
+   * @expectedDeprecation Drupal\Tests\editor\Unit\BcEditor::settingsForm is deprecated since version 8.3.x. Rename the implementation 'buildConfigurationForm'. See https://www.drupal.org/node/2819753
+  +  @expectedDeprecation Drupal\Tests\editor\Unit\BcEditor::settingsFormValidate is deprecated since version 8.3.x. Rename the implementation 'validateConfigurationForm'. See https://www.drupal.org/node/2819753
+  +  @expectedDeprecation Drupal\Tests\editor\Unit\BcEditor::settingsFormSubmit is deprecated since version 8.3.x. Rename the implementation 'submitConfigurationForm'. See https://www.drupal.org/node/2819753
    */

Fixed this on commit (+ to *), and updated the change record.

Committed 012b0c7 and pushed to 8.8.x. Thanks!

  • catch committed 012b0c7 on 8.8.x 
    Issue #3002933 by alexpott, Mile23, Wim Leers: Fix "The Drupal\editor\...

  • alexpott committed ce3fa51 on 8.8.x 
    Issue follow-up #3002933 by alexpott: Fix "The Drupal\editor\Plugin\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Status: Fixed » Closed (fixed)

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