Follow up for #1971384: [META] Convert page callbacks to controllers and #1913618: Convert EntityFormControllerInterface to extend FormInterface

Whilst we have _entity_form for our defaults in routing, it can't be expected to deal with entity add callbacks, as these need to create a psuedo entity first and the keys they need to set vary (eg menu items may need a plid) or are dynamic (eg node/add/type and block/add/custom_block_type).

This leaves the controller methods for callbacks like block/add/custom_block_type #1978166: Convert block/add/{%custom_block_type} to Controller needing to call entity_get_form from their controllers which breaks our OO goal.

This issue tracks moving entity_get_form to Drupal\Core\Entity\EntityManager::getForm()

Related

#1982984: Create Drupal::entityManager for improved DX

Files: 
CommentFileSizeAuthor
#42 entity-get-form-1982980-38.patch21.86 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,092 pass(es).
[ View ]
#42 interdiff.txt2.33 KBParisLiakos
#36 entity-get-form-1982980-36.patch21.98 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#36 interdiff.txt2.56 KBParisLiakos
#34 entity-get-form-1982980-43.patch22.54 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#30 entity-get-form-1982980-30.patch18.05 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,010 pass(es).
[ View ]
#28 entity-get-form-00-1982980.28.patch18.03 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-get-form-00-1982980.28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 entity-get-form-00-1982980.26.patch18.66 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,599 pass(es).
[ View ]
#24 entity-get-form-00-1982980.24.patch19.74 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,691 pass(es).
[ View ]
#20 entity-get-form-00-1982980.20.patch20.01 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,788 pass(es).
[ View ]
#17 entity-get-form-00-1982980.17.patch20.82 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,196 pass(es).
[ View ]
#16 entity-get-form-00-1982980.16.patch20.85 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,662 pass(es).
[ View ]
#15 entity-get-form-00-1982980.15.patch20.85 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,692 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#12 entity-get-form-00-1982980.12.interdiff.txt1.43 KBlarowlan
#12 entity-get-form-00-1982980.12.patch20.64 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,533 pass(es).
[ View ]
#10 drupal-1982980-10.patch21.24 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,996 pass(es).
[ View ]
#7 drupal-1982980-7.patch21.54 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,509 pass(es).
[ View ]
#4 entity-get-form-00-1982980.3.interdiff.txt3.61 KBlarowlan
#4 entity-get-form-00-1982980.3.patch22.16 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,409 pass(es).
[ View ]
#1 entity-get-form-00-1982980.1.patch22.15 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,372 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

larowlan’s picture

Assigned:larowlan» Unassigned
Status:Active» Needs review
StatusFileSize
new22.15 KB
FAILED: [[SimpleTest]]: [MySQL] 55,372 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

How goes it bot?

Status:Needs review» Needs work

The last submitted patch, entity-get-form-00-1982980.1.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldTest.phpundefined
@@ -65,7 +65,7 @@ function testUpdateAllowedValues() {
+    $form = Drupal::service('plugin.manager.entity')->getForm($entity);

@@ -91,7 +91,7 @@ function testUpdateAllowedValues() {
+    $form = Drupal::service('plugin.manager.entity')->getForm($entity);

@@ -99,7 +99,7 @@ function testUpdateAllowedValues() {
+    $form = Drupal::service('plugin.manager.entity')->getForm($entity);

@@ -120,7 +120,7 @@ function testUpdateAllowedValues() {
+    $form = Drupal::service('plugin.manager.entity')->getForm($entity);

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationFormTest.phpundefined
@@ -81,7 +81,7 @@ function testEntityFormLanguage() {
+    Drupal::service('plugin.manager.entity')->getForm($node, 'default', $form_state);

Needs to be \Drupal (that's the two failures)

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new22.16 KB
PASSED: [[SimpleTest]]: [MySQL] 55,409 pass(es).
[ View ]
new3.61 KB

yeah, sorry picked that up too
one of those days.

ParisLiakos’s picture

this is awesome:)
+1000

andypost’s picture

dawehner’s picture

StatusFileSize
new21.54 KB
PASSED: [[SimpleTest]]: [MySQL] 55,509 pass(es).
[ View ]

Updated the patch

ParisLiakos’s picture

Status:Needs review» Needs work
+++ b/core/includes/entity.incundefined
@@ -487,11 +487,12 @@ function entity_form_submit(EntityInterface $entity, $operation = 'default', &$f
function entity_get_form(EntityInterface $entity, $operation = 'default', array $form_state = array()) {

why dont we remove this entirely?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -200,4 +200,30 @@ public function getAccessController($entity_type) {
+   * @param string $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. Defaults 'default'.

Docs are messed up:) some of documentation for $operation is in $form_state

tim.plunkett’s picture

We cannot remove it until all of the 'page callback' => 'entity_get_form', routes are converted.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new21.24 KB
PASSED: [[SimpleTest]]: [MySQL] 55,996 pass(es).
[ View ]

There are currently 7 different menu entries which still use entity_get_form.

Here is just a simple rerole + fix of the good point in #8.

ParisLiakos’s picture

Issue tags:+Needs reroll

We cannot remove it until all of the 'page callback' => 'entity_get_form', routes are converted.

right i forgot those page callbacks..

this needs a reroll and while at it:

+++ b/core/includes/entity.incundefined
@@ -487,11 +487,12 @@ function entity_form_submit(EntityInterface $entity, $operation = 'default', &$f
+ * @deprecated Use Drupal::entityManager()->getForm() instead
+ *   or _entity_form from a routing.yml file instead of a page callback.

lets fix the double "instead"

larowlan’s picture

StatusFileSize
new20.64 KB
PASSED: [[SimpleTest]]: [MySQL] 55,533 pass(es).
[ View ]
new1.43 KB

Re-roll and fixes #11 as well as a reference to Drupal::service('plugin.manager.entity') that snuck back in.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

Great!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

No longer applies

curl https://drupal.org/files/entity-get-form-00-1982980.12.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 21135  100 21135    0     0  16854      0  0:00:01  0:00:01 --:--:-- 19587
error: patch failed: core/modules/options/lib/Drupal/options/Tests/OptionsFieldTest.php:65
error: core/modules/options/lib/Drupal/options/Tests/OptionsFieldTest.php: patch does not apply
ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new20.85 KB
FAILED: [[SimpleTest]]: [MySQL] 55,692 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

reroll, also replaced one more instance is user.admin.inc, probably just sneaked in

ParisLiakos’s picture

StatusFileSize
new20.85 KB
PASSED: [[SimpleTest]]: [MySQL] 55,662 pass(es).
[ View ]
+++ b/core/modules/user/user.admin.incundefined
@@ -479,7 +479,7 @@ function user_admin_roles_list() {
+  return \Drupal::entityManager()->getForm($role);

not needed prefix

ParisLiakos’s picture

StatusFileSize
new20.82 KB
PASSED: [[SimpleTest]]: [MySQL] 56,196 pass(es).
[ View ]

rerolled, replacing more calls that sneaked in the meanwhile

aspilicious’s picture

Shouldn't we move "entity_form_state_defaults" to the controller too? Or is that for another issue?

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Good idea in general, though we don't replace entity_form_submit so far, so let's go with this.

ParisLiakos’s picture

StatusFileSize
new20.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,788 pass(es).
[ View ]

no longer applies

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

The last submitted patch, entity-get-form-00-1982980.20.patch, failed testing.

tim.plunkett’s picture

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

#20: entity-get-form-00-1982980.20.patch queued for re-testing.

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community
ParisLiakos’s picture

StatusFileSize
new19.74 KB
PASSED: [[SimpleTest]]: [MySQL] 56,691 pass(es).
[ View ]

chasing HEAD

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -317,7 +317,7 @@ public function save() {
-      $original = \Drupal::service('plugin.manager.entity')
+      $original = \Drupal::entityManager()

Unrelated change.

And the patch no longer applies

curl https://drupal.org/files/entity-get-form-00-1982980.24.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 20213  100 20213    0     0  15323      0  0:00:01  0:00:01 --:--:-- 17871
error: patch failed: core/modules/block/block.admin.inc:84
error: core/modules/block/block.admin.inc: patch does not apply
error: patch failed: core/modules/config/tests/config_test/config_test.module:86
error: core/modules/config/tests/config_test/config_test.module: patch does not apply
error: patch failed: core/modules/node/node.pages.inc:24
error: core/modules/node/node.pages.inc: patch does not apply
ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new18.66 KB
PASSED: [[SimpleTest]]: [MySQL] 57,599 pass(es).
[ View ]

removed unrelated change and reroll

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

back to rtbc

ParisLiakos’s picture

StatusFileSize
new18.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-get-form-00-1982980.28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

one more reroll after admin/people view went in

Status:Reviewed & tested by the community» Needs work

The last submitted patch, entity-get-form-00-1982980.28.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new18.05 KB
PASSED: [[SimpleTest]]: [MySQL] 58,010 pass(es).
[ View ]
ParisLiakos’s picture

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

we are close to 10 rerolls here:P

alexpott’s picture

Issue tags:+Needs reroll

Ho hum... here we go again...

curl https://drupal.org/files/entity-get-form-1982980-30.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 18484  100 18484    0     0  13193      0  0:00:01  0:00:01 --:--:-- 15454
error: patch failed: core/modules/block/custom_block/custom_block.pages.inc:55
error: core/modules/block/custom_block/custom_block.pages.inc: patch does not apply
error: patch failed: core/modules/node/node.pages.inc:92
error: core/modules/node/node.pages.inc: patch does not apply
alexpott’s picture

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

Status:Needs work» Needs review
StatusFileSize
new22.54 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

conflicted with #1978166: Convert block/add/{%custom_block_type} to Controller
i injected the plugin manager instead and put the request in the controller method since i was touching it

tim.plunkett’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -16,52 +16,29 @@
-    $this->customBlockStorage = $custom_block_storage;
-    $this->customBlockTypeStorage = $custom_block_type_storage;
...
+    $this->entityManager = $entity_manager;

@@ -73,7 +50,7 @@ public function __construct(Request $request, EntityStorageControllerInterface $
-    $types = $this->customBlockTypeStorage->load();
+    $types = $this->entityManager->getStorageController('custom_block_type')->load();

If we need the entity manager in addition, so be it. But this is not the right direction to go in for the storage controllers

ParisLiakos’s picture

StatusFileSize
new2.56 KB
new21.98 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

yeap, i stand corrected

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

The last submitted patch, entity-get-form-1982980-36.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI, +Needs reroll, +FormInterface

#36: entity-get-form-1982980-36.patch queued for re-testing.

larowlan’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -38,6 +52,8 @@ public static function create(ContainerInterface $container) {
+    $this->customBlockStorage = $entity_manager->getStorageController('custom_block');
+    $this->customBlockTypeStorage = $entity_manager->getStorageController('custom_block_type');

shouldn't we be injecting these too?

tim.plunkett’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -40,28 +40,21 @@ class CustomBlockController implements ControllerInterface {
-      $entity_manager->getStorageController('custom_block'),
-      $entity_manager->getStorageController('custom_block_type')
...
-  public function __construct(Request $request, EntityStorageControllerInterface $custom_block_storage, EntityStorageControllerInterface $custom_block_type_storage) {
-    $this->request = $request;
-    $this->customBlockStorage = $custom_block_storage;
-    $this->customBlockTypeStorage = $custom_block_type_storage;
+  public function __construct(PluginManagerInterface $entity_manager) {
+    $this->customBlockStorage = $entity_manager->getStorageController('custom_block');
+    $this->customBlockTypeStorage = $entity_manager->getStorageController('custom_block_type');
+    $this->entityManager = $entity_manager;

Getting rid of the Request is correct, but not the other two

Status:Needs review» Needs work

The last submitted patch, entity-get-form-1982980-36.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new2.33 KB
new21.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,092 pass(es).
[ View ]

k lets just swap request with entity manager

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

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

tim.plunkett’s picture

Status:Needs work» Needs review

#42: entity-get-form-1982980-38.patch queued for re-testing.

Status:Needs review» Needs work

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

ParisLiakos’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI, +Needs reroll, +FormInterface

#42: entity-get-form-1982980-38.patch queued for re-testing.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

Sixth time's the charm?

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed c4b6658 and pushed to 8.x. Thanks!

I don't think this needs a change notice as we haven't removed entity_get_form() yet...

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.