Support from Acquia helps fund testing for Drupal Acquia logo

Comments

juampynr’s picture

Status: Needs work » Active
Issue tags: -WSCCI-conversion
FileSize
7.72 KB

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

juampynr’s picture

Assigned: Unassigned » juampynr
Status: Active » Needs review
FileSize
7.96 KB

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

juampynr’s picture

Issue tags: +WSCCI-conversion

Tagging.

dawehner’s picture

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

juampynr’s picture

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.

dawehner’s picture

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.

dawehner’s picture

Status: Needs review » Needs work

.

juampynr’s picture

Thanks for the remainder! Working on it...

juampynr’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
8.51 KB

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.

dawehner’s picture

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.

Berdir’s picture

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

tim.plunkett’s picture

Status: Needs review » Needs work

This needs a reroll for FormBase

tim.plunkett’s picture

This needs a reroll for FormBase

juampynr’s picture

Status: Needs work » Needs review
FileSize
8.42 KB
1.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.

xjm’s picture

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.

dawehner’s picture

Status: Active » Needs review
Issue tags: +WSCCI-conversion
FileSize
8.51 KB

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.

juampynr’s picture

Status: Needs work » Needs review
FileSize
4.58 KB

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

juampynr’s picture

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

juampynr’s picture

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

juampynr’s picture

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.

juampynr’s picture

Status: Needs work » Needs review
juampynr’s picture

Berdir’s picture

pfrenssen’s picture

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.

juampynr’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
920 bytes

Thanks! Here it is.

pfrenssen’s picture

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.

juampynr’s picture

Status: Needs work » Needs review
FileSize
3.73 KB

Re-rolling.

juampynr’s picture

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.

tim.plunkett’s picture

  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.

swentel’s picture

Issue summary: View changes
Status: Needs work » Closed (fixed)

This happened in a different issue.