Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#32 | drupal-convert-field_test_entity_nested_form-1978890-32.patch | 5.01 KB | juampynr |
#31 | drupal-convert-field_test_entity_nested_form-1978890-31.patch | 3.73 KB | juampynr |
#29 | interdiff.txt | 920 bytes | juampynr |
#29 | drupal-convert-field_test_entity_nested_form-1978890-29.patch | 4.72 KB | juampynr |
#23 | interdiff.txt | 2.17 KB | juampynr |
Comments
Comment #1
juampynr CreditAttribution: juampynr commentedFirst pass. Pending to review tests and fix bugs in my patch.
Comment #2
juampynr CreditAttribution: juampynr commentedAdded converter options to to field_test.routing.yml in order to load the entities. Now giving it a try against tests.
Comment #3
juampynr CreditAttribution: juampynr commentedTagging.
Comment #4
dawehnerIs this method call also injectable?
We could just use an injected entity storage controller instead if you like
Comment #5
juampynr CreditAttribution: juampynr commentedSorry @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.
Comment #6
dawehnerentity_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.
Comment #7
dawehner.
Comment #8
juampynr CreditAttribution: juampynr commentedThanks for the remainder! Working on it...
Comment #9
juampynr CreditAttribution: juampynr commentedHere it is. Let's see if tests pass.
Comment #11
dawehnerThis should be better something like @var \Drupal\Core...
I am wondering what happens if you don't use drupal_set_title but set #title on &$form instead.
Comment #12
BerdirUps.
Comment #13
tim.plunkettThis needs a reroll for FormBase
Comment #14
tim.plunkettThis needs a reroll for FormBase
Comment #15
juampynr CreditAttribution: juampynr commentedRe-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:
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:
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.
Comment #17
xjmThanks 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.
Comment #18
dawehnerThe syntax in the routing yml file seemed to have changed.
Comment #20
juampynr CreditAttribution: juampynr commentedOK, rewrote the patch so it satisfies Step 1 of https://groups.drupal.org/node/315498.
Comment #21
juampynr CreditAttribution: juampynr commentedNote 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.
Comment #22
juampynr CreditAttribution: juampynr commentedThanks to #2090581: Custom form controllers cannot make use of upcasted parameters I will provide a new patch that will use param converters.
Comment #23
juampynr CreditAttribution: juampynr commentedFinally. Ready for review.
Comment #25
juampynr CreditAttribution: juampynr commented#23: drupal-convert-field_test_entity_nested_form-1978890-22.patch queued for re-testing.
Comment #26
juampynr CreditAttribution: juampynr commented#23: drupal-convert-field_test_entity_nested_form-1978890-22.patch queued for re-testing.
Comment #27
Berdir#23: drupal-convert-field_test_entity_nested_form-1978890-22.patch queued for re-testing.
Comment #28
pfrenssenDoesn't seem to be used.
You can omit this if it is empty.
Comment #29
juampynr CreditAttribution: juampynr commentedThanks! Here it is.
Comment #30
pfrenssenNitpick: group the Drupal classes together, and order alphabetically.
This is unrelated? Probably a copy/paste artifact.
The $entity_N variables are not optional.
Comment #31
juampynr CreditAttribution: juampynr commentedRe-rolling.
Comment #32
juampynr CreditAttribution: juampynr commentedAddressed suggested changes in #30 except optional variables since that changes the method signature and gives the following error when running NestedFormTest:
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.
Comment #34
tim.plunkettWhy bother changing this? :) Also we have left requirements last everywhere else.
Delete this
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.
Comment #35
swentel CreditAttribution: swentel commentedThis happened in a different issue.