Part of #2072251: [meta] Modernize forms to use FormBase

Updated: Comment #N

Problem/Motivation

Now that #2059245: Add a FormBase class containing useful methods is in, I was looking at old forms that have empty validateForm() methods, or still use Drupal::service() or t().

Proposed resolution

Convert existing FormInterface forms to extend FormBase

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#5 editor_forms-2077973-5.patch2.13 KBplopesc
#3 editor_forms-2077973-3.patch5.76 KBplopesc
#3 interdiff.txt485 bytesplopesc
#1 editor_forms-2077973-1.patch5.76 KBplopesc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Active » Needs review
FileSize
editor_forms-2077973-1.patch5.76 KB

Hello

With this patch, editor module forms inherits from FormBase and removed usages of t(), replaced by $this->t().

Regards.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

Just one minor nitpick:

+++ b/core/modules/editor/lib/Drupal/editor/Form/EditorImageDialog.php
@@ -7,19 +7,18 @@
+

Extraneous newline.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs work » Needs review
FileSize
interdiff.txt485 bytes
editor_forms-2077973-3.patch5.76 KB

Removing empty line...

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Postponed

Looks good to go, but this will conflict with #2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images, which also fixes this for EditorImageDialog, which has been dragging along for months. I want to avoid yet another reroll.

So: postponing this issue on that issue. Sorry.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Postponed » Needs review
FileSize
editor_forms-2077973-5.patch2.13 KB

Re-rolling once #2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images has been committed.

Wim Leers’s picture

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

Code looks good. Makes EditorLinkDialog's code look like EditorImageDialog's (see #4).

Manually tested, all continues to work well.

RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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