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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

disasm’s picture

Assigned: Unassigned » disasm

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

disasm’s picture

Component: other » forms system
Status: Active » Needs review
FileSize
2.9 KB

path module

tim.plunkett’s picture

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

tim.plunkett’s picture

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

disasm’s picture

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).

disasm’s picture

helps if I upload the right patch...

tim.plunkett’s picture

  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

disasm’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
9.62 KB
34.42 KB

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.

effulgentsia’s picture

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?

tim.plunkett’s picture

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)

disasm’s picture

Status: Needs work » Needs review
FileSize
2.93 KB
34.49 KB

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.
disasm’s picture

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

disasm’s picture

effulgentsia’s picture

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.

disasm’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

disasm’s picture

addressing #19 and #20.

disasm’s picture

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.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +FormInterface, +WSCCI-conversion
tim.plunkett’s picture

Assigned: disasm » tim.plunkett
FileSize
40.39 KB

@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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.7 KB
46.61 KB

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

jibran’s picture

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

alexpott’s picture

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.