Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#32 drupal-convert-field_test_entity_nested_form-1978890-32.patch5.01 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to create checkout database.
[ View ]
#31 drupal-convert-field_test_entity_nested_form-1978890-31.patch3.73 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 58,680 pass(es).
[ View ]
#29 interdiff.txt920 bytesjuampy
#29 drupal-convert-field_test_entity_nested_form-1978890-29.patch4.72 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 58,643 pass(es).
[ View ]
#23 interdiff.txt2.17 KBjuampy
#23 drupal-convert-field_test_entity_nested_form-1978890-22.patch4.96 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 58,732 pass(es).
[ View ]
#20 drupal-convert-field_test_entity_nested_form-1978890-20.patch4.58 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 59,233 pass(es).
[ View ]
#18 field_test-1978890-18.patch8.51 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,714 pass(es), 63 fail(s), and 37 exception(s).
[ View ]
#1 drupal-convert-field_test_entity_nested_form-1978890-1.patch7.72 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,003 pass(es), 63 fail(s), and 57 exception(s).
[ View ]
#15 interdiff.txt1.61 KBjuampy
#15 drupal-convert-field_test_entity_nested_form-1978890-15.patch8.42 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,347 pass(es), 65 fail(s), and 12 exception(s).
[ View ]
#9 drupal-convert-field_test_entity_nested_form-1978890-9.patch8.51 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,314 pass(es), 65 fail(s), and 12 exception(s).
[ View ]
#9 interdiff.txt3.88 KBjuampy
#2 drupal-convert-field_test_entity_nested_form-1978890-2.patch7.96 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 57,958 pass(es).
[ View ]

Comments

Status:Needs work» Active
Issue tags:-WSCCI-conversion
StatusFileSize
new7.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,003 pass(es), 63 fail(s), and 57 exception(s).
[ View ]

First pass. Pending to review tests and fix bugs in my patch.

Assigned:Unassigned» juampy
Status:Active» Needs review
StatusFileSize
new7.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,958 pass(es).
[ View ]

Added converter options to to field_test.routing.yml in order to load the entities. Now giving it a try against tests.

Issue tags:+WSCCI-conversion

Tagging.

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.phpundefined
@@ -0,0 +1,114 @@
+    $form_state['form_display'] = entity_get_form_display($entity_1->entityType(), $entity_1->bundle(), 'default');
...
+    $form_state['form_display'] = entity_get_form_display($entity_1->entityType(), $entity_1->bundle(), 'default');

Is this method call also injectable?

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.phpundefined
@@ -0,0 +1,114 @@
+    $entity_1 = entity_create('entity_test', $form_state['values']);
+    field_attach_extract_form_values($entity_1, $form, $form_state);
...
+    $entity_2 = entity_create('entity_test', $form_state['values']['entity_2']);
...
+    $entity_1 = entity_create('entity_test', $form_state['values']);
...
+    $entity_2 = entity_create('entity_test', $form_state['values']['entity_2']);

We could just use an injected entity storage controller instead if you like

Sorry @dawehner but I do not know how to inject a function in the constructor. Shall I make entity_get_form_display() and entity_create() a service?

If you can point me to an example I will be happy to look into it.

entity_create() is just an alias for $entity_manager->getStorageController('entity_test')->create so you could inject the entity manager here.
entity_get_form_display seems also just to use entity_load.

Status:Needs review» Needs work

.

Thanks for the remainder! Working on it...

Status:Needs work» Needs review
StatusFileSize
new3.88 KB
new8.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,314 pass(es), 65 fail(s), and 12 exception(s).
[ View ]

Here it is. Let's see if tests pass.

Status:Needs review» Needs work

The last submitted patch, drupal-convert-field_test_entity_nested_form-1978890-9.patch, failed testing.

Status:Needs work» Needs review
  1. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php
    @@ -0,0 +1,123 @@
    +   * @var Drupal\Core\Entity\EntityManager

    This should be better something like @var \Drupal\Core...

  2. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php
    @@ -0,0 +1,123 @@
    +
    +    drupal_set_message(t('test_entities @id_1 and @id_2 have been updated.', array('@id_1' => $entity_1->id(), '@id_2' => $entity_2->id())));

    I am wondering what happens if you don't use drupal_set_title but set #title on &$form instead.

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php
@@ -0,0 +1,123 @@
+ * Displays banned IP addresses.
...
+class EntityNestedForm implements FormInterface, ControllerInterface {

Ups.

Status:Needs review» Needs work

This needs a reroll for FormBase

This needs a reroll for FormBase

Status:Needs work» Needs review
StatusFileSize
new8.42 KB
FAILED: [[SimpleTest]]: [MySQL] 58,347 pass(es), 65 fail(s), and 12 exception(s).
[ View ]
new1.61 KB

Re-rolled and made the suggested changes.

We are getting a lot of failing assertions and warnings because the parameter converters are not working (@dawehner, you gave me a tip about these at Dublin).

The patch define the following param converters:

--- /dev/null
+++ b/core/modules/field/tests/modules/field_test/field_test.routing.yml
@@ -0,0 +1,10 @@
+field_test_entity_nested_form:
+  pattern: 'test-entity/nested/{entity_1}/{entity_2}'
+  defaults:
+    _form: '\Drupal\field_test\Form\EntityNestedForm'
+  requirements:
+    _permission: 'administer entity_test content'
+  options:
+    converters:
+      entity_1: 'entity_test'
+      entity_2: 'entity_test'

Those two entities are not being converted to instances of entity_test. I have also reviewed the Upcasting tests and they use a testing module which implements them in a different way:

paramconverter_test_node_user_user:
  pattern: '/paramconverter_test/test_node_user_user/{node}/{foo}/{user}'
  defaults:
    _content: '\Drupal\paramconverter_test\TestControllers::testUserNodeFoo'
  requirements:
    _access: 'TRUE'
  options:
    parameters:
      foo:
        type: 'entity:user'

I have tried the above without luck either.

Any tips? I could dig deeper in the flow to see what is happening but I already spend a couple hours yesterday and my neurons got fried.

Status:Needs review» Needs work

The last submitted patch, drupal-convert-field_test_entity_nested_form-1978890-15.patch, failed testing.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Status:Active» Needs review
Issue tags:+WSCCI-conversion
StatusFileSize
new8.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,714 pass(es), 63 fail(s), and 37 exception(s).
[ View ]

The syntax in the routing yml file seemed to have changed.

Status:Needs review» Needs work

The last submitted patch, field_test-1978890-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.58 KB
PASSED: [[SimpleTest]]: [MySQL] 59,233 pass(es).
[ View ]

OK, rewrote the patch so it satisfies Step 1 of https://groups.drupal.org/node/315498.

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php
@@ -0,0 +1,69 @@
+    $entity_1 = entity_load('entity_test', $entity_1);

Note that I had to load entities manually since param converters are just supported by controllers and entity forms, but not by custom forms (see the logic at http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...).

Is this in purpose? Looks like a regression to me since in Drupal 7 form callbacks could have loaders in their paths.

Thanks to #2090581: Custom form controllers cannot make use of upcasted parameters I will provide a new patch that will use param converters.

StatusFileSize
new4.96 KB
PASSED: [[SimpleTest]]: [MySQL] 58,732 pass(es).
[ View ]
new2.17 KB

Finally. Ready for review.

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

The last submitted patch, drupal-convert-field_test_entity_nested_form-1978890-22.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.phpundefined
@@ -0,0 +1,68 @@
+use Symfony\Component\HttpFoundation\Request;

Doesn't seem to be used.

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.phpundefined
@@ -0,0 +1,68 @@
+  /**
+   * Constructs an EntityNestedForm object.
+   */
+  public function __construct() {
+  }
+

You can omit this if it is empty.

Status:Needs work» Needs review
StatusFileSize
new4.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,643 pass(es).
[ View ]
new920 bytes

Thanks! Here it is.

Status:Needs review» Needs work

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.phpundefined
@@ -0,0 +1,61 @@
+use Drupal\Core\Form\FormBase;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Entity\EntityInterface;

Nitpick: group the Drupal classes together, and order alphabetically.

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.phpundefined
@@ -0,0 +1,61 @@
+/**
+ * Displays banned IP addresses.
+ */

This is unrelated? Probably a copy/paste artifact.

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.phpundefined
@@ -0,0 +1,61 @@
+  public function buildForm(array $form, array &$form_state, EntityInterface $entity_1 = NULL, EntityInterface $entity_2 = NULL) {

The $entity_N variables are not optional.

Status:Needs work» Needs review
StatusFileSize
new3.73 KB
PASSED: [[SimpleTest]]: [MySQL] 58,680 pass(es).
[ View ]

Re-rolling.

StatusFileSize
new5.01 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to create checkout database.
[ View ]

Addressed suggested changes in #30 except optional variables since that changes the method signature and gives the following error when running NestedFormTest:

Fatal error: Declaration of Drupal\field_test\Form\EntityNestedForm::buildForm() must be compatible with Drupal\Core\Form\FormInterface::buildForm(array $form, array &$form_state) in /home/juampy/projects/drupal8/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php on line 17

Here I am also removing core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/FieldTestForm.php since it just contains the form builder, which is being implemented already by core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php

Too many changes, interdiff not really useful here.

Status:Needs review» Needs work

The last submitted patch, drupal-convert-field_test_entity_nested_form-1978890-32.patch, failed testing.

  1. +++ b/core/modules/field/tests/modules/field_test/field_test.routing.yml
    @@ -2,12 +2,12 @@ field_test.entity_nested_form:
    +  requirements:
    +    _permission: 'administer entity_test content'
    ...
    -  requirements:
    -    _permission: 'administer entity_test content'

    Why bother changing this? :) Also we have left requirements last everywhere else.

  2. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php
    @@ -0,0 +1,61 @@
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static();
    +  }

    Delete this

  3. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Form/EntityNestedForm.php
    @@ -0,0 +1,61 @@
    +    module_load_include('entity.inc', 'field_test');
    ...
    +    module_load_include('entity.inc', 'field_test');
    ...
    +    field_test_entity_nested_form_submit($form, $form_state);

    So, this is not an improvement. We're just trading 1 module_load_include for 3.

    (By the way, if left as is this should have used form_load_include once, but it doesn't matter)

    Let's just actually remove the deprecated functions now.