Convert EntityFormControllerInterface to extend FormInterface

+++ b/core/includes/entity.inc
@@ -451,9 +427,10 @@ function entity_form_id(EntityInterface $entity, $operation = 'default') {
-  $form_state['build_info']['args'] = array($entity);
+  $controller->setEntity($entity);

It seems that most of the changes in this patch are originated by this hunk, which is not strictly required to implement FormInterface. Although it'll improve DX I'm afraid it has also the potential for introducing misalignments between the form controller and the form state entity references. This seems a bit out of scope here. We decided not to wrap the form state into the controller for similar reasons, see #1499596-96: Introduce a basic entity form controller.

Do you think this change is necessary here?

Btw, this is the issue the linked post is referring to: #1728812: Prevent entity form controllers from being cached twice (in $form and $form_state).

So this hardcodes a form controller to a route, which is not what we want, but is all I can do right now. Just a proof of concept.

Isn't this out of scope wrt the straight conversion?

I've also completely removed $form_state['entity'], so that there is only one place storing it. Either use $this->entity within the controller, or $form_state['controller']->getEntity() outside it (which should have been done anyway, since currently the location its stored in the form_state was variable!).

Removing $form_state['entity'] and unifying the way we retrieve an entity in the form workflow was one of my original goals when working on the initial EFC patch, so I'm largely +1 on this. Moreover, in light of #5 this looks like the only sensible thing to do, although it makes the form controller a stateful object. I'd like to see what @fago thinks of this, since it was one of the points we discussed more.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewCloneFormController.php
@@ -12,6 +12,14 @@
+    return entity_build_form($this);

I guess we should use an injected entity manger to retrieve the needed controller in these cases. Similar issue: #1919322: entity_load_unchanged() should be part of the storage controller.

Sure, we can punt on actually using this correctly from routes, and continue to call entity_get_form(). It needs to happen, if you want a follow-up that's fine.

No strong preference on this, it's just that I can do a quite solid review of the form controller related code, but I have nfi of what the Views stuff is doing :)

I'm not sure what you mean by injecting the manager, the referenced issue mentions it but doesn't explain it and the patch doesn't seem to do it.

I mean that by calling entity_*_controller() we are relying on drupal_container(), instead of having a properly injected entity manager. This makes unit testing stuff harder/dirtier.

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -23,30 +23,70 @@ class EntityFormController implements EntityFormControllerInterface {
+    $form_id = $entity_type;
+    if ($bundle != $entity_type) {
+      $form_id = $bundle . '_' . $form_id;
+    }
+    if ($this->operation != 'default') {
+      $form_id = $form_id . '_' . $this->operation;
+    }
+    return $form_id . '_form';

Suppose better to have entity type the leading name - {entity}_form__{bundle} and bundle (sub type) as context like hook_forms() usage

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -330,4 +330,29 @@ public function getAccessController($entity_type) {
+  public function getForm($entity, $operation = 'default') {
+    if (is_string($entity)) {
+      $entity = $this->getStorageController($entity)->create(array());

Suppose D8MI would like to pass lang_code here.
So third argument could be used for default values and having $entity as mixed parameter looks bad

+++ /dev/nullundefined
@@ -1,28 +0,0 @@
-class UserRouteController extends ContainerAware {

great!

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Routing/ViewsUIController.phpundefined
@@ -76,19 +76,6 @@ public function listing() {
-  public function add() {
-    drupal_set_title(t('Add new view'));

@@ -200,20 +187,6 @@ public function ajaxOperation(ViewStorageInterface $view, $op, Request $request)
-   */
-  public function cloneForm(ViewStorageInterface $view) {
-    drupal_set_title(t('Clone of @human_name', array('@human_name' => $view->getHumanName())));

@@ -266,22 +239,6 @@ public function edit(ViewUI $view, $display_id = NULL) {
-  public function preview(ViewUI $view, $display_id = NULL) {
-    return entity_get_form($view, 'preview', array('display_id' => $display_id));

+++ b/core/modules/views/views_ui/views_ui.routing.ymlundefined
@@ -91,13 +97,16 @@ views_ui.edit.display:
 views_ui.preview:
-  pattern: '/admin/structure/views/view/{view}/preview/{display_id}'
+  pattern: '/admin/structure/views/view/{entity}/preview/{display_id}'
   options:
+    converters:
+      entity: 'view'
     tempstore:
-      view: 'views'
+      entity: 'views'
   defaults:
-    _controller: 'views_ui.controller:preview'
+    _controller: 'plugin.manager.entity:getForm'
     display_id: NULL
+    operation: 'preview'

Nice clean-up but settings title from form controller is bad

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewAddFormController.phpundefined
@@ -17,16 +16,25 @@
+  public function init(array &$form_state) {
+    parent::init($form_state);
+
+    drupal_set_title(t('Add new view'));

This makes embedding of the form much harder. Not sure it's nice idea to affect page level elements from controllers!

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewFormControllerBase.phpundefined
@@ -25,25 +24,26 @@
-  public function build(array $form, array &$form_state, EntityInterface $entity) {
-    if (isset($form_state['display_id'])) {
-      $this->displayID = $form_state['display_id'];
+  public function init(array &$form_state) {
+    parent::init($form_state);
+
+    if ($display_id = drupal_container()->get('request')->attributes->get('display_id')) {

Are you sure to move this to request level? needs code comment and mention in change notice

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -0,0 +1,64 @@
+   * This would mean the edit form controller for the node entity should be use.

*used

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -0,0 +1,64 @@
+    // If no operation is provided, use 'default'.
+    $arg .= '.default';
+    list ($entity, $operation) = explode('.', $arg);

This is weird. Clever, but weird.

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -0,0 +1,64 @@
+    elseif (is_string($entity)) {
+      $entity = $manager->getStorageController($entity)->create(array());

Doesn't $entity have to be a string here?

+++ b/core/lib/Drupal/Core/Form/BaseFormInterface.phpundefined
@@ -0,0 +1,25 @@
+  /**
+   * Returns a string identifying the base form.
+   *
+   * @return string|false
+   *   The string identifying the base form or FALSE if this is not a base form.
+   *
+   *   The unique string identifying the form.
+   */

So forms that aren't base forms can implement the BaseFormInterface?

Also, "The unique string identifying the form." seems redundant.

+++ b/core/lib/Drupal/Core/Form/BaseFormInterface.phpundefined
@@ -0,0 +1,25 @@
+interface BaseFormInterface extends FormInterface {

This just seems odd to me. I mean I get what you're doing here, but the name convention makes me think that FormInterface should extend BaseFormInterface. Maybe this could be BaseFormIdInterface? Does this need to be disconnected for the EntityFormControllerInterface?

$arg .= '.default';

Also not really ok with that line. It sounds like you aren't either so I'm just ++ing changing that line.

In general the updates to the implementations all look very sane. The actual changes to the EntityFormController itself give me pause since the updating process of the entity on the actual class is a sticky thing. It LOOKS right to me, but I don't feel comfortable being the person to rtbc this for that specific reason alone. The rest of it looks pretty good to me and like a good improvement. My one other question is just that it appears we now have a buildForm(), build() and form() methods and that gives me pause as well. Can you explain that a bit?

Eclipse

ok, but entity_get_form() does pass 'default' as the second parameter already in a default. Is this strictly necessary? If so I'll happily drop the topic. It's a pretty minor point and what you're doing is definitely not prone to error, so I don't have complaints other than it's sort of weird looking.

Ok, so build() is gone, buildForm() is in its place, and the call to form() is permanent? or a temporary shim you want to revisit. Just trying to be clear here, I'm cool either way.

Definitely let's not put the actions thing on FormInterface, I think it's working perfectly without that right now. :-)

Eclipse

Just a quick skim through...

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.php
@@ -0,0 +1,64 @@
+  /**
+   * {@inheritdoc}
+   *
+   * Due to reflection, the argument must be named $_entity_form.
+   */
+  public function content(Request $request, $_entity_form) {
+    return parent::content($request, $_entity_form);
+  }

This seems redundant...

+++ b/core/lib/Drupal/Core/Form/BaseFormInterface.php
@@ -0,0 +1,25 @@
+/**
+ * Provides an interface for a Form that has a base form ID.
+ */
+interface BaseFormInterface extends FormInterface {
+
+  /**
+   * Returns a string identifying the base form.
+   *
+   * @return string|false
+   *   The string identifying the base form or FALSE if this is not a base form.
+   *
+   *   The unique string identifying the form.
+   */
+  public function getBaseFormID();
+

I don't even understand the purpose of this... That may help in coming up with a better name.

+++ b/core/lib/Drupal/Core/HtmlFormController.php
@@ -73,9 +61,34 @@ public function content(Request $request, $_form) {
+  protected function getFormObject(Request $request, $arg) {

It took me 3 reads to figure out that $arg was actually the name of a form. Can we call it something more descriptive, like, $form? $form_def? Something with "form" in it?

This looks awesome! i think it is very RTBCable.
Just two reallyy minor points:

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -23,30 +23,70 @@ class EntityFormController implements EntityFormControllerInterface {
+  public function setOperation($operation) {
+    if ($operation) {
+      $this->operation = $operation;
+    }

why is the conditional needed?

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -0,0 +1,65 @@
+  /**
+   * {@inheritdoc}
+   *
+   * Due to reflection, the argument must be named $_entity_form. The parent
+   * method has $request and $_form, but the parameter must match the route.
+   */

This inheritdoc wont be parsed, cause it contains additional description..can we have it old style like below?

Edit, Nvm

a lot better now:)

Needs a reroll

error: patch failed: core/modules/shortcut/lib/Drupal/shortcut/ShortcutFormController.php:53
error: core/modules/shortcut/lib/Drupal/shortcut/ShortcutFormController.php: patch does not apply

Rerolled.

#44: entity-form-interface-1913618-44.patch queued for re-testing.

back to rtbc

Couple of minor nits otherwise looks great...

+++ b/core/includes/entity.incundefined
@@ -468,9 +444,10 @@ function entity_form_id(EntityInterface $entity, $operation = 'default') {
   $controller = drupal_container()->get('plugin.manager.entity')->getFormController($entity->entityType(), $operation);
-  $form_state['build_info']['callback'] = array($controller, 'build');
-  $form_state['build_info']['base_form_id'] = $entity->entityType() . '_form';
-  $form_state['build_info']['args'] = array($entity);
+  $controller->setEntity($entity);

Lets use Drupal::service() here

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -23,30 +23,71 @@ class EntityFormController implements EntityFormControllerInterface {
     // During the initial form build, add the entity to the form state for use
     // during form building and processing. During a rebuild, use what is in the
     // form state.
-    if (!$this->getEntity($form_state)) {
-      $this->init($form_state, $entity);
+    if (!isset($form_state['controller'])) {
+      $this->init($form_state);
     }

The comment no longer seems to be true...

Committed 620d497 and pushed to 8.x. Thanks!

Follow up bug: #1983164: Entity Forms in ajax requests don't find the route

I updated the change notice to fix ::getEntity() calls in a couple of examples.

So what is better approach now:
1) $form_state['controller']->getEntity()?
2) $this->entity?

Mension about it https://drupal.org/node/1734556/revisions/view/2722135/2722977

Looks good to me as well, thanks, just replaced FormInterface with EntityFormControllerInterface :)

Edit: Oh, well, I added another example ;)