Convert field_test_entity_nested_form to a Controller

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

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

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.

.

Thanks for the remainder! Working on it...

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

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.

This needs a reroll for FormBase

This needs a reroll for FormBase

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.

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.

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

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.

Finally. Ready for review.

#23: drupal-convert-field_test_entity_nested_form-1978890-22.patch queued for re-testing.

#23: drupal-convert-field_test_entity_nested_form-1978890-22.patch queued for re-testing.

#23: drupal-convert-field_test_entity_nested_form-1978890-22.patch queued for re-testing.

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

Thanks! Here it is.

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

Re-rolling.

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.

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.

This happened in a different issue.