Problem/Motivation

In #3252386: Use PHP attributes instead of doctrine annotations we added support for attribute based plugin discovery.
As part of that issue we converted block and action plugins.

This issue is to convert \Drupal\editor\Annotation\Editorplugins to use Attributes.

Proposed resolution

  1. Add a class to represent the new Attribute - Example
  2. Update the plugin manager constructor to include both the attribute and annotation class names - example
  3. Convert all plugins that use the annotation to use the new attribute - example

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 core-convert-editor-plugin-discovery-to-attributes-3421011-2.patch5.16 KBkalpanajaiswal
#2 core-convert-ditor-plugin-discovery-to-attributes-3421011-2.patch5.92 KBkalpanajaiswal

Issue fork drupal-3421011

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan created an issue. See original summary.

kalpanajaiswal’s picture

kalpanajaiswal commented
StatusFileSize
new core-convert-ditor-plugin-discovery-to-attributes-3421011-2.patch5.92 KB

Converted Editor plugin discovery to attributes.

kalpanajaiswal’s picture

kalpanajaiswal commented
StatusFileSize
new core-convert-editor-plugin-discovery-to-attributes-3421011-2.patch5.16 KB

Converted Editor plugin discovery to attributes.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs work
Issue tags: +Needs reroll

Thanks! 😊 Could you please convert this to a merge request, for much faster test results?

mstrelan made their first commit to this issue’s fork.

mstrelan opened merge request !6776

mstrelan’s picture

mstrelan commented
Issue tags: -Needs reroll

MR is up, added feedback and suggestions to the MR.

quietone made their first commit to this issue’s fork.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Accepted the other suggestion, to fix the spelling error.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs subsystem maintainer review

I'm the subsystem maintainer and would like to be able to sign off on this. Love where this is going! 😄👍

sorlov made their first commit to this issue’s fork.

sorlov’s picture

sorlov commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to phpcs and stan errors.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs subsystem maintainer review

Because \Drupal\editor\Plugin\EditorPluginInterface::getJSSettings() and \Drupal\editor\Plugin\EditorPluginInterface::getLibraries() inappropriately are using the concrete Editor class in their signature rather than EditorInterface, an awkward aliasing-upon-using was necessary 😥

As far as I'm concerned, this is ready (except for the one comment nit that is yet to be applied).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Related issues: +#2442255: Changing text formats on a field makes it impossible to edit.

I can reproduce the test failure locally. It's in a test that was added >9 years ago, in #2442255: Changing text formats on a field makes it impossible to edit..

quietone’s picture

quietone commented
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

The sole remaining unresolved thread is still not resolved, and it's about fixing a big bug in the current MR: https://git.drupalcode.org/project/drupal/-/merge_requests/6776#note_278485

Ruturaj Chaubey made their first commit to this issue’s fork.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

This looks ready to me now!

But one test is failing:

Drupal\FunctionalJavascriptTests\Ajax\AjaxTest::testAjaxFocus
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'edit-textfield-3'
+'edit-textfield-2'

🤔 Re-testing…

smustgrave’s picture

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

Appears all feedback has been addressed

All instances replaced

Tests are green :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed e715970 and pushed to 11.x. Thanks!
Committed c84ae09 and pushed to 10.3.x. Thanks!

  • alexpott committed c84ae09d on 10.3.x 
    Issue #3421011 by sorlov, Wim Leers, quietone, kalpanajaiswal, mstrelan...

  • alexpott committed e7159707 on 11.x 
    Issue #3421011 by sorlov, Wim Leers, quietone, kalpanajaiswal, mstrelan...

Status: Fixed » Closed (fixed)

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