Updated: Comment #N

Problem/Motivation

Converting all form callbacks that haven't been converted yet to form controllers

Proposed resolution

create FormBase for each form, return drupal_get_form() of the old form callback in buildForm()

Remaining tasks

User interface changes

API changes

Follow-up from #1971384: [META] Convert page callbacks to controllers.

Files: 
CommentFileSizeAuthor
#27 form-non-test-2089671-27.patch46.61 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,661 pass(es).
[ View ]
#27 interdiff.txt6.7 KBtim.plunkett
#25 form-non-test-2089671-25.patch40.39 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,125 pass(es), 34 fail(s), and 2 exception(s).
[ View ]
#22 drupal8.forms-system.2089671-22.patch47.58 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,855 pass(es).
[ View ]
#22 interdiff.txt6.73 KBdisasm
#21 drupal8.forms-system.2089671-21.patch47.58 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#21 interdiff.txt22.78 KBdisasm
#16 drupal8.forms-system.2089671-16.patch40.91 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,841 pass(es).
[ View ]
#16 interdiff.txt8 KBdisasm
#15 drupal8.forms-system.2089671-15.patch34.47 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#15 interdiff.txt3.97 KBdisasm
#14 drupal8.forms-system.2089671-13.patch34.49 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#14 interdiff.txt2.93 KBdisasm
#10 drupal8.forms-system.2089671-10.patch34.42 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 59,085 pass(es), 32 fail(s), and 6 exception(s).
[ View ]
#10 interdiff.txt9.62 KBdisasm
#8 drupal8.forms-system.2089671-8.patch34.6 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 54,347 pass(es), 631 fail(s), and 20,313 exception(s).
[ View ]
#8 interdiff.txt5.69 KBdisasm
#6 drupal8.forms-system.2089671-3.patch34.63 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,560 pass(es), 678 fail(s), and 26,265 exception(s).
[ View ]
#5 drupal8.forms-system.2089671-2.patch2.9 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#2 drupal8.forms-system.2089671-2.patch2.9 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,198 pass(es).
[ View ]

Comments

Assigned:Unassigned» disasm

assigning to myself to get this started. Will unassign before leaving the country in two days.

Component:other» forms system
Status:Active» Needs review
StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,198 pass(es).
[ View ]

path module

This issue should be just for 'page callback' => 'drupal_get_form'.

And you might want to just call drupal_get_form in a vanilla Controller, and skip FormBase completely for now.

StatusFileSize
new2.9 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

First pass at this. Most of this is untested, so expecting some failures. I know there's one in book I was getting locally. Shortcut module still needs access checkers (access: TRUE currently).

StatusFileSize
new34.63 KB
FAILED: [[SimpleTest]]: [MySQL] 57,560 pass(es), 678 fail(s), and 26,265 exception(s).
[ View ]

helps if I upload the right patch...

  1. +++ w/core/modules/book/book.routing.yml
    @@ -50,3 +50,18 @@ book.admin_edit:
    +    parameters:
    +      node:
    +        type: 'entity:node'

    Why is this needed?

  2. +++ w/core/modules/book/lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.php
    @@ -0,0 +1,55 @@
    +    return static::KILL;

    I don't know of many other places we use KILL.

  3. +++ w/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.php
    @@ -0,0 +1,72 @@
    +class ContentTranslationManageAccessCheck implements AccessCheckInterface {

    Should be StaticAccessCheckInterface

  4. +++ w/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.php
    @@ -0,0 +1,72 @@
    +      $route_requirements = $route->getRequirements();
    +      $operation = $route_requirements['_access_content_translation_manage'];

    use getRequirement() instead

  5. +++ w/core/modules/shortcut/lib/Drupal/shortcut/Form/ShortcutForm.php
    @@ -0,0 +1,46 @@
    +  public function edit($menu_link) {
    ...
    +    $menu_link = menu_link_load($menu_link);
    ...
    +  public function add($shortcut_set) {
    ...
    +    $shortcut_set = shortcut_set_load($shortcut_set);
    ...
    +  public function overview($user) {
    ...
    +    $user = user_load($user);

    If you type hint this, it should work just fine.

  6. +++ w/core/modules/shortcut/shortcut.routing.yml
    @@ -46,3 +46,32 @@ shortcut.set_customize:
    +  options:
    +    _access_checks: 'ALL'
    +  requirements:
    +    _access: 'TRUE'
    ...
    +  options:
    +    _access_checks: 'ALL'
    +  requirements:
    +    _access: 'TRUE'

    the ALL is overkill

  7. +++ w/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccessCheck.php
    @@ -0,0 +1,51 @@
    +class UpdateManagerAccessCheck implements AccessCheckInterface {

    StaticAccessCheckInterface

StatusFileSize
new5.69 KB
new34.6 KB
FAILED: [[SimpleTest]]: [MySQL] 54,347 pass(es), 631 fail(s), and 20,313 exception(s).
[ View ]

Addresses all but #6. Shortcut doesn't have access checks written yet, and I'm planning on combining a mix of access checks and permission checks for those possibly, so leaving the _access_checks: 'ALL' there for now.

I'm not sure if this will work with #1 removed, but we'll see. Last time I tried to access an upcasted argument in an access checker without a param converter specified, it failed pretty bad, but that was a while ago. That might be functioning now.

Status:Needs review» Needs work

The last submitted patch, drupal8.forms-system.2089671-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.62 KB
new34.42 KB
FAILED: [[SimpleTest]]: [MySQL] 59,085 pass(es), 32 fail(s), and 6 exception(s).
[ View ]

most of those failures are from pattern -> path in routing.yml. Here's a reroll on that. Also fixing applies -> appliesTo since we're using Static access checker.

Status:Needs review» Needs work

The last submitted patch, drupal8.forms-system.2089671-10.patch, failed testing.

And you might want to just call drupal_get_form in a vanilla Controller, and skip FormBase completely for now.

How long is "for now"? If it's just to get this patch passing first, then convert from _content to _form still in this issue prior to commit, then ok. But is there a reason to commit form routes to HEAD that incorrectly use _content instead of _form?

By for now, I mean, commit this as is (once it's green). The goal here is to remove all page callbacks as quickly as possible, not to kill drupal_get_form().

At least that was my understanding of Phase 1 of these new conversions. Properly chopping up the magically-named validate and submit callbacks and making them methods on a class and handing getFormID should be part of Phase 2 (especially if we do decide to go ahead with #2089593: Have FormBase auto-generate form IDs, so you don't need to implement getFormID() on all forms)

Status:Needs work» Needs review
StatusFileSize
new2.93 KB
new34.49 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fixes content translation tests. That means the single failure in book.module and fixing shortcut module are all that remain.

@effulgentsia This is a crappy hack, we know that. There's a few reasons for doing it this way:

  1. Patches that were "almost" RTBC will reroll fairly easily. The only affected parts should be hook_menu and routing.yml, as well as deleting the false controller file.
  2. The goal of this approach is to kill the legacy menu router as fast as possible. If we can get these committed quickly, that means work can continue on proper conversions.
  3. This approach gets the "hard" things taken care of for converters. Once the access checks and route subscribers are in place, most conversions should be fairly straight forward. A number of patches were getting held up by failing access checkers and route subscribers, not to mention having to reroll those when changes like currentUser() service were committed.

StatusFileSize
new3.97 KB
new34.47 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

fixes book. Needed _access_checks -> _access_mode. Also, renamed it something that looks more like an access checker in the route.

StatusFileSize
new8 KB
new40.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,841 pass(es).
[ View ]

I agree with most of the reasoning in #13 and #14.

+++ w/core/modules/book/lib/Drupal/book/Form/BookForm.php
@@ -0,0 +1,25 @@
+class BookForm {
+  /**
+   * Wraps book_remove_form().
+   *
+   * @see book_remove_form()
+   */
+  public function remove(EntityInterface $node) {
+    module_load_include('pages.inc', 'book');
+    return drupal_get_form('book_remove_form', $node);
+  }
+
+}

I don't mind this crappy hack at all.

+++ w/core/modules/book/book.routing.yml
@@ -50,3 +50,15 @@ book.admin_edit:
+    _content: '\Drupal\book\Form\BookForm::remove'

But I'm concerned about this existing in HEAD for any longer than absolutely necessary. I think contrib developers will already be struggling a bit to understand the differences between _controller, _content, _form, and _entity_form without us giving them incorrect and contradictory examples.

But I don't want to derail this issue. Would you instead be ok with me rolling a patch after this one gets committed that immediately converts these to _form, and therefore, the classes to extend FormBase, and have wrapper methods for validateForm() and submitForm() that call back to the procedural ones, and getting that follow up patch in quickly, before doing the full phase 2?

Patches that were "almost" RTBC will reroll fairly easily. The only affected parts should be hook_menu and routing.yml, as well as deleting the false controller file.

My suggestion wouldn't derail that, would it? You would still be able to replace the temporary form classes with the real ones from those patches.

I'm fine with the suggestion in #17 as long as it doesn't put a hold on doing proper conversions like this set of issues is doing.

Overall, looks good! Some of those access checks look ugly as hell, but content_translation has bizarre needs.

  1. +++ w/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
    @@ -0,0 +1,79 @@
    +  }
    +}

    Missing blank line

  2. +++ w/core/modules/language/lib/Drupal/language/Form/LanguageForm.php
    @@ -0,0 +1,23 @@
    +class LanguageForm {
    +  /**
    +++ w/core/modules/locale/lib/Drupal/locale/Form/LocaleForm.php
    @@ -0,0 +1,43 @@
    +class LocaleForm {
    +  /**
    +++ w/core/modules/shortcut/lib/Drupal/shortcut/Form/ShortcutForm.php
    @@ -0,0 +1,47 @@
    +class ShortcutForm {
    +  /**

    Here, and elsewhere, missing a blank line

  3. +++ w/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetEditAccessCheck.php
    @@ -0,0 +1,42 @@
    +      return !isset($shortcut_set) || $shortcut_set == shortcut_current_displayed_set() ? static::ALLOW : static::DENY;
    +    }
    +  }
    +++ w/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetSwitchAccessCheck.php
    @@ -0,0 +1,50 @@
    +      return static::ALLOW;
    +    }
    +  }
    +

    Missing a return static::DENY; at the end.

Oh, also. The temporary methods should have @todo, and the callbacks you wrap should be marked @deprecated

StatusFileSize
new22.78 KB
new47.58 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

addressing #19 and #20.

StatusFileSize
new6.73 KB
new47.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,855 pass(es).
[ View ]

typo on deprecated.

Status:Needs review» Needs work
Issue tags:-WSCCI, -FormInterface, -WSCCI-conversion

The last submitted patch, drupal8.forms-system.2089671-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI, +FormInterface, +WSCCI-conversion

Assigned:disasm» tim.plunkett
StatusFileSize
new40.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,125 pass(es), 34 fail(s), and 2 exception(s).
[ View ]

@disasm went on vacation. Rerolling after the non-test non-form one went in.

Status:Needs review» Needs work

The last submitted patch, form-non-test-2089671-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.7 KB
new46.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,661 pass(es).
[ View ]

Messed up the content_translation merge, and fixed a bug with user/reset

Status:Needs review» Reviewed & tested by the community

I think it is good to go. I am unable to find anything objectionable in the patch.

+++ b/core/modules/book/book.routing.yml
@@ -50,3 +50,15 @@ book.admin_edit:
+    _access_mode: 'ALL'

I just RTBC #2063401: Replace the default _access_checks(access mode) with ALL instead of ANY

Status:Reviewed & tested by the community» Fixed

Committed 523c8b6 and pushed to 8.x. Thanks!

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