Support from Acquia helps fund testing for Drupal Acquia logo

Comments

CB’s picture

Assigned: Unassigned » CB
CB’s picture

Using #1978946: Convert comment_edit_page() to a Controller as an example here. (And adding this comment as a reminder to myself.)

amateescu’s picture

Status: Active » Closed (won't fix)

entity_get_form() is not a form controller, it's just a procedural helper :) And it's going away in #1982980: Move entity_get_form to Drupal\Core\Entity\EntityManager::getForm() anyway..

CB’s picture

Yep, been chatting with larowlan about this today.

oenie’s picture

Hi,

Since you guys seem to know what's going on on the entity-side of things, could this issue be of the same type:

#1987698 : Convert entity_view() to a new style controller

As with entity_get_form, entity_view is in entity.inc

Any ideas ?
Just looking to see if i can help a hand on conversion issues, preferably clear ones that need to be done :)

amateescu’s picture

Yeah, that's the same situation. Thanks for bringing it up :)

larowlan’s picture

Title: Convert entity_get_form() to a new style controller » Convert all references to entity_get_form() as page callbacks to _entity_form w/ corresponding routing.yml entries
Component: entity system » menu system
Status: Closed (won't fix) » Active

Even though entity_get_form is going OO
We still need to convert all entries in hook_menu where the page callback is entity_get_form to use _entity_form and routing.
Updating issue title.

CB’s picture

Component: menu system » entity system
Status: Active » Closed (won't fix)
FileSize
8.25 KB

Patch attached.

CB’s picture

Component: entity system » menu system
Status: Closed (won't fix) » Needs review

Woops, fixing statuses.

Status: Needs review » Needs work

The last submitted patch, convert-entity_get_form-to-routing-1987692-7.patch, failed testing.

larowlan’s picture

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -53,3 +53,17 @@ aggregator_page_last:
+  pattern: 'aggregator/sources/{aggregator_feed}/configure'
...
+  pattern: 'admin/config/services/aggregator/edit/feed/{aggregator_feed}'

+++ b/core/modules/menu/menu.routing.ymlundefined
@@ -25,3 +25,10 @@ menu_delete_menu:
+  pattern: 'admin/structure/menu/item/{menu_link}/edit'

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -0,0 +1,13 @@
+  pattern: 'taxonomy/term/{taxonomy_term}/edit'
...
+  pattern: 'admin/structure/taxonomy/manage/{taxonomy_vocabulary}/edit'

+++ b/core/modules/user/user.routing.ymlundefined
@@ -54,3 +54,10 @@ user_role_delete:
+  pattern: 'user/{user}/edit'

needs leading /

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
--- /dev/null
+++ b/core/modules/entity/lib/Drupal/entity/Controller/EntityController.phpundefined

+++ b/core/modules/entity/lib/Drupal/entity/Controller/EntityController.phpundefined
+++ b/core/modules/entity/lib/Drupal/entity/Controller/EntityController.phpundefined
@@ -0,0 +1,56 @@

@@ -0,0 +1,56 @@
+<?php
+/**
+* @file
+* Contains \Drupal\entity\Controller\EntityController.
+*/
+
+namespace Drupal\entity\Controller;
+
+use Drupal\Core\ControllerInterface;
+use Drupal\Core\Entity\EntityInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+
+/**
+* Controller routines for entity routes.
+*/
+class EntityController implements ControllerInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * Constructs a EntityController object.
+   */
+  public function __construct() {
+  }
+
+  /**
+   * Returns the built and processed entity form for the given entity.
+   *
+   * @param EntityInterface $entity
+   *   The entity to be created or edited.
+   * @param $operation
+   *   (optional) The operation identifying the form variation to be returned.
+   * @param array $form_state
+   *   (optional) An associative array containing the current state of the form.
+   *   Use this to pass additional information to the form, such as the langcode.
+   *   @code
+   *   $form_state['langcode'] = $langcode;
+   *   $form = entity_get_form($entity, 'default', $form_state);
+   *   @endcode
+   *
+   * @return
+   *   The processed form for the given entity and operation.
+   */
+  public function getForm(EntityInterface $entity, $operation = 'default', array $form_state = array()) {
+    var_dump(get_class($entity)); exit;
+    $form_state += entity_form_state_defaults($entity, $operation);
+    $form_id = $form_state['build_info']['callback_object']->getFormID();
+    return drupal_build_form($form_id, $form_state);
+  }
+
+}

this looks like left-over from another patch?

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -0,0 +1,13 @@
+    _entity_form: taxonomy_term.default
...
+    _entity_access: taxonomy_term.update
...
+    _entity_form: taxonomy_vocabulary.default
...
+    _entity_access: taxonomy_vocabulary.update

+++ b/core/modules/user/user.routing.ymlundefined
@@ -54,3 +54,10 @@ user_role_delete:
+    _entity_form: user.profile
...
+    _entity_access: user.update

I think you need to single quote these?

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -0,0 +1,13 @@
\ No newline at end of file

needs a new line

The fails are down to missing include files. eg the cancel form is in the user.pages.inc but we're not loading that now. Those forms should really become their own page callbacks, there is one for the vocabulary one already, not sure about the user cancel.

+++ b/core/modules/menu/menu.routing.ymlundefined
@@ -25,3 +25,10 @@ menu_delete_menu:
+    _entity_form: 'menu.default'

should be menu_link.default - will fix a swathe of the fails

CB’s picture

this looks like left-over from another patch?

Woops, I started writing Controllers in a typical approach to WSCCI conversion. I forgot to delete it before I created a patch. :)

I think you need to single quote these?

I had them single quoted but found a whole bunch of instances where they weren't quoted (user.routing.yml for example.) So I assumed that they shouldn't be. Fixed now.

should be menu_link.default - will fix a swathe of the fails

Sweet!

Thanks larowlan!

CB’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, convert-entity_get_form-to-routing-1987692-12.patch, failed testing.

CB’s picture

Status: Needs work » Needs review
FileSize
6.76 KB
10.72 KB
ParisLiakos’s picture

Status: Needs review » Postponed

looks like a great cleanup:)
lets wait for #1946456: Convert taxonomy_term_confirm_delete() and taxonomy_vocabulary_confirm_delete() to the new form interface thats RTBC so we can get rid of the @todo as well

tim.plunkett’s picture

Status: Postponed » Needs work
becw’s picture

The patch in #15 is very out of date. I'm going to steal this issue and re-rolling the patch; here is just the user edit portion. More coming shortly.

becw’s picture

Assigned: becw » Unassigned
Status: Needs work » Needs review
FileSize
7.8 KB

The original patch had routes for aggregator feed, menu link, taxonomy term and vocabulary, and user entity edit forms, but the menu link route updates have already been applied as part of some other issue.

Here's an updated patch that updates the remaining uses of entity_get_form() in hook_menu().

tim.plunkett’s picture

Wow, thanks for jumping on this!

  1. +++ b/core/modules/user/lib/Drupal/user/ProfileFormController.php
    @@ -41,4 +41,19 @@ public function save(array $form, array &$form_state) {
    +   * Submit method for the 'Cancel account' button.
    

    Technically this should be something like "Provides a submit handler for the 'Cancel account' button."

  2. +++ b/core/modules/user/lib/Drupal/user/ProfileFormController.php
    @@ -41,4 +41,19 @@ public function save(array $form, array &$form_state) {
    +    $query = Drupal::request()->query;
    

    We should make the buildForm store the request, and reuse it here.

  3. +++ b/core/modules/user/lib/Drupal/user/ProfileFormController.php
    @@ -41,4 +41,19 @@ public function save(array $form, array &$form_state) {
    +    $account = $form_state['controller']->getEntity();
    +    $form_state['redirect'] = array("user/" . $account->id() . "/cancel", array('query' => $destination));
    

    Instead of all this to get $account, you can just use $this->entity, that pattern was for external code. Also, can we swap the double quotes for single quotes?

Status: Needs review » Needs work

The last submitted patch, convert-entity_get_form-to-routing-1987692-19.patch, failed testing.

becw’s picture

Assigned: Unassigned » becw
FileSize
8.59 KB

I've made the changes you suggested, and I also took the opportunity to replace $GLOBAL['user'] with $this->request->attributes->get('_account');.

Testbot reported some issues too, but I haven't fixed those yet. I'll leave this as "needs work" until I finish addressing those.

becw’s picture

The failing tests are at least in part due to issues with the tests themselves:

  • Drupal\taxonomy\Tests\TermTest includes a test that ensures that entity_get_form() does not accept additional arguments from the URL. This test isn't relevant anymore since the new routing system doesn't route these paths to the term edit page:
        /* Drupal\taxonomy\Tests\TermTest, lines 351-354 */
        // Check that the term edit page does not try to interpret additional path
        // components as arguments for entity_get_form().
        $this->drupalGet('taxonomy/term/' . $term->id() . '/edit/' . $this->randomName());
        $this->assertResponse(200, 'The taxonomy term edit menu item ensured appropriate arguments were passed to its page callback.');
    
  • Drupal\user\Tests\UserCancelTest's testUserCancelInvalid() seems to have a bug where it doesn't confirm the account cancellation correctly.

Additionally, the new request dependency in ProfielFormController was missing the "use" statement at the top of the file.

Here's an updated patch, with an interdiff to the patch from the re-roll in #19.

Status: Needs review » Needs work

The last submitted patch, convert-entity_get_form-to-routing-1987692-23.patch, failed testing.

becw’s picture

Assigned: becw » Unassigned
Status: Needs work » Needs review
FileSize
3.57 KB
9.6 KB

There were some changes to aggregator.module in the past few hours. Here's a re-roll, again with an interdiff against the patch in #19.

Status: Needs review » Needs work

The last submitted patch, convert-entity_get_form-to-routing-1987692-25-interdiff.patch, failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
9.6 KB

I get different test failures when I run these tests locally, so I'm not quite sure what's going on. I'm going to post the same patch once more, without the mis-named interdiff.

Status: Needs review » Needs work

The last submitted patch, convert-entity_get_form-to-routing-1987692-25.patch, failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
9.88 KB

Here's an update that fixes dependency injection now that FormBase is in (#2059245: Add a FormBase class containing useful methods).

becw’s picture

timplunkett pointed out in IRC that this should use FormBase::getCurrentUser().

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This looks perfect now. Thanks so much @becw!

grep -r "entity_get_form[^_]" *
core/includes/entity.inc: *   $form = entity_get_form($entity, 'default', $form_state);
core/includes/entity.inc:function entity_get_form(EntityInterface $entity, $operation = 'default', array $form_state = array()) {
tim.plunkett’s picture

Note, this has the same hunk that is being discussed in #1987898-40: Convert user_view_page() to a new style controller

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
9.67 KB

Straight reroll.

alexpott’s picture

Issue tags: +Needs reroll

Patch no longer applies

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
xjm’s picture

Poor @tim.plunkett :)

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
8.25 KB
catch’s picture

Issue tags: -WSCCI-conversion

#38: entity-form-1987692-38.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, entity-form-1987692-38.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.19 KB

Patch context conflict, nothing more.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Comitted and pushed to 8.x. Thanks!

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