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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Active » Needs review
FileSize
22.15 KB

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
FileSize
22.16 KB
3.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

FileSize
21.54 KB

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
FileSize
21.24 KB

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

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
FileSize
20.85 KB

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

ParisLiakos’s picture

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

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

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
ParisLiakos’s picture

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

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
FileSize
18.66 KB

removed unrelated change and reroll

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

ParisLiakos’s picture

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
FileSize
18.05 KB
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
FileSize
22.54 KB

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

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
FileSize
2.33 KB
21.86 KB

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.