Problem/Motivation

Core entity forms are currently very similar in their basic functionality but each entity type replicates more or less the same code to implement more or less the same logic. This is true for the main create/edit forms as for the other operations such as the deletion/deletion confirmation forms. This unnecessarily bloats the core codebase and makes maintaining it more difficult. Moreover changes to the common logic need to be manually propagated to all core entity types and contrib has no way to reuse the logic implemented by core entity forms.
Additionally it's impossible to tell whether a form is an entity form in a reliable way when altering it, because there is no standardization enforced among them, just some guideline. This makes handling them in a generic fashion really difficult.

Proposed resolution

By introducing a common entity form controller based on the Entity Property API we will able to generate a basic entity form without needing any additional code from the modules defining the entities. However those will be able to subclass the entity form controller and perform any needed alteration to its behavior.

An entity form controller is an object responsible for handling the entity form workflow: from building the form to performing validation and submission tasks. The base implementation provides the common logic and will exploit the Entity Property API to generalize most of its tasks, so that only minimal custom logic will need to be provided in the entity-specific subclasses.

Entity form controllers are defined in the entity info as storage ones, but they differ from those in the sense that an entity can have multiple form controllers, each one responsible for managing a different operation form. This way we can have, for instance, different controllers for the regular create/edit form and the deletion/deletion confirmation ones. Regardless of the operation, the controller defines standard methods to build the entity from the submitted values and retrieve it from the form state, thus providing a more consistent and reliable API to code altering the entity form workflow. AAMOF the controller itself is stored into the form state and can be retrieved by any form handler/callback. This is also an easy way to spot if we are dealing with an entity form.

The current solution performs a few small tweaks to the Form API, which are @chx-approved, to allow for any callable to become a form handler/callback. This way class methods can be used as form handlers and overridden by subclasses, which can customize the form submission workflow to fit their needs or just get the default behavior. By default different callbacks are assigned to the various form actions, thus allowing the controller to provide separately overridable methods corresponding to them. This way subclassess can decide to customize only the validation/submission handlers they need to.

Remaining tasks

Here is a list of follow up issues. The nested ones may or may not deserve a dedicated issue, we will see while working on the main one.

List available also at:

http://drupal.org/project/issues/search/drupal?issue_tags=Entity%20forms

User interface changes

No UI change.

API changes

The form state build info support a new 'callback' key that can be used to explicitly set a form builder callback. Moreover now any callable can be used as a form handler/callback.

The recommended way to get the entity being operated on is no longer $form_state[$entity_type] (or similar) but $form_state['controller']->getEntity($form_state).

Some new API functions have been introduced:

  • entity_form_controller() can be used to instantiate a new form controller class for the given operation
  • entity_form_state_defaults() provides a form state stub with the build info properly populated to build an entity form
  • entity_form_id() computes the form id for the given entity type/operation couple
  • entity_get_form() returns a rendered entity form for the given operation
  • entity_form_submit() submits an entity form with the given form state
CommentFileSizeAuthor
#125 entity_forms-1499596-125.interdiff.do_not_test.patch421 bytesplach
#125 entity_forms-1499596-125.patch198.44 KBplach
#123 entity_forms-1499596-123.interdiff.do_not_test.patch10.07 KBplach
#123 entity_forms-1499596-123.patch198.44 KBplach
#106 entity_forms-1499596-106.interdiff.do_not_test.patch752 bytesplach
#106 entity_forms-1499596-106.patch200.56 KBplach
#104 entity_forms-1499596-104.patch200.54 KBplach
#100 entity_forms-1499596-100.interdiff.do_not_test.patch1.54 KBplach
#100 entity_forms-1499596-100.patch200.52 KBplach
#96 entity_form.interdiff.txt42.45 KBfago
#96 entity_form.patch200.64 KBfago
#94 entity_forms-1499596-94.interdiff.do_not_test.patch1.28 KBplach
#94 entity_forms-1499596-94.patch198.72 KBplach
#93 entity_forms-1499596-93.interdiff.do_not_test.patch730 bytesplach
#93 entity_forms-1499596-93.patch198.38 KBplach
#92 entity_forms-1499596-92.patch198.23 KBplach
#91 entity_forms-1499596-91.interdiff.do_not_test.patch939 bytesplach
#91 entity_forms-1499596-91.patch459.86 KBplach
#88 entity_forms-1499596-88.interdiff.do_not_test.patch31.7 KBplach
#88 entity_forms-1499596-88.patch198.08 KBplach
#85 entity_forms-1499596-85.interdiff.do_not_test.patch717 bytesplach
#85 entity_forms-1499596-85.patch197.1 KBplach
#83 entity_forms-1499596-83.interdiff.do_not_test.patch13.24 KBplach
#83 entity_forms-1499596-83.patch197.1 KBplach
#80 entity_forms-1499596-80.patch197.02 KBplach
#77 entity_forms-1499596-77.patch196.67 KBplach
#70 entity_forms-1499596-70.interdiff.do_not_test.patch37.61 KBplach
#70 entity_forms-1499596-70.patch196.53 KBplach
#75 entity_forms-1499596-75.interdiff.do_not_test.patch2.09 KBplach
#75 entity_forms-1499596-75.patch197.03 KBplach
#72 entity_forms-1499596-72.interdiff.do_not_test.patch790 bytesplach
#72 entity_forms-1499596-72.patch196.68 KBplach
#67 entity_forms-1499596-68.patch179.33 KBplach
#67 entity_forms-1499596-68.interdiff.do_not_test.patch131.3 KBplach
#65 entity_forms-1499596-65.patch75.88 KBplach
#61 entity_forms-1499596-61.patch75.99 KBplach
#58 entity_forms-1499596-58.interdiff.do_not_test.patch17.94 KBplach
#58 entity_forms-1499596-58.patch75.99 KBplach
#55 entity_forms-1499596-55.interdiff.do_not_test.patch4.44 KBplach
#55 entity_forms-1499596-55.patch73.74 KBplach
#51 entity_forms-1499596-51.interdiff.do_not_test.patch623 bytesplach
#51 entity_forms-1499596-51.patch72.75 KBplach
#50 entity_forms-1499596-50.patch72.62 KBplach
#50 entity_forms-1499596-50.interdiff.do_not_test.patch22.59 KBplach
#46 entity_forms-1499596-46.patch72.17 KBplach
#44 entity_forms-1499596-44.patch72.08 KBplach
#42 entity_forms-1499596-42.patch70.32 KBplach
#40 entity_forms-1499596-40.patch69.69 KBplach
#38 entity_forms-1499596-38.patch67.54 KBplach
#35 entity_forms-1499596-35.patch60.57 KBplach
#30 entity_forms-1499596-30.interdiff.do_not_test.patch5.52 KBplach
#30 entity_forms-1499596-30.patch59.47 KBplach
#28 entity_forms-1499596-28.patch60.6 KBplach
#22 entity_forms-1499596-22.interdiff.do_not_test.patch1.36 KBplach
#22 entity_forms-1499596-22.patch60.56 KBplach
#18 entity_forms-1499596-18.interdiff.do_not_test.patch38.06 KBplach
#18 entity_forms-1499596-18.patch60.55 KBplach
#14 entity_forms-1499596-14.interdiff.do_not_test.patch6.34 KBplach
#14 entity_forms-1499596-14.patch39.24 KBplach
#13 entity_forms-1499596-13.interdiff.do_not_test.patch4.95 KBplach
#13 entity_forms-1499596-13.patch39.95 KBplach
#7 entity_forms-1499596-7.patch39.14 KBplach
#3 entity_forms-1499596-3.patch37.97 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Assigned: Unassigned » dawehner

Just started and looking forward to do some work during the flight.

plach’s picture

Assigned: dawehner » plach

Actually I already have a semi-working patch. I forgot to assign this to myself.

plach’s picture

Assigned: plach » Unassigned
Status: Active » Needs review
FileSize
37.97 KB

Here is a first attempt. The goal of this patch is providing the basic infrastructure, AAMOF the form builder/handler functions are not actually migrated to the new system. This in itself involves a deep refactoring that might deserve its own issue. Let's see how many failures we have here.

plach’s picture

A bunch of things to fix once the testbot has completed the review.

+++ b/core/lib/Drupal/Core/Entity/FormController.php
@@ -0,0 +1,154 @@
+   * Retuerns the actualform array to be built.

typos

+++ b/core/lib/Drupal/Core/Entity/FormController.php
@@ -0,0 +1,154 @@
+   * Returns the code identifying the active form language. ¶

+++ b/core/modules/entity/entity.module
@@ -428,6 +430,112 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+  // If a non-existing context has been specified stop. ¶

Trailing whitespaces

+++ b/core/modules/entity/entity.class.inc
@@ -277,6 +282,15 @@ class Entity implements EntityInterface {
+    // @todo Move entity_edit() here as soon as all core entities are subclasses
+    // of the Entity class.

Nope, entity_edit() is useful as a page callback.

+++ b/core/modules/node/node.pages.inc
@@ -323,7 +322,7 @@ function node_form($form, &$form_state, $node) {
+//     '#submit' => array('node_form_submit'),

@@ -346,13 +345,12 @@ function node_form($form, &$form_state, $node) {
+//   $form['#validate'][] = 'node_form_validate';

Fix these

+++ b/core/modules/taxonomy/taxonomy.module
@@ -143,6 +146,9 @@ function taxonomy_entity_info() {
+        'default' => '\\Drupal\\Module\\Taxonomy\\VocabularyFormController',

Wrong indentation

+++ b/core/modules/user/user.admin.inc
@@ -11,7 +11,8 @@ function user_admin($callback_arg = '') {
+      //drupal_get_form('user_register_form');

Remove this one

2 days to next Drupal core point release.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-3.patch, failed testing.

Gábor Hojtsy’s picture

#1188388: Entity translation UI in core is related, in fact this one is a sub-issue.

plach’s picture

Status: Needs work » Needs review
FileSize
39.14 KB

Rerolled #4 and catched-up with entity OOP-fication.

Tests still to be fixed, let's see where failures are. FWIW the patch is performing pretty well on manual testing.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-7.patch, failed testing.

tim.plunkett’s picture

Title: Introduce a basic entity from controller » Introduce a basic entity form controller
sun’s picture

Category: task » feature
+++ b/core/modules/comment/comment.module
@@ -1649,19 +1659,7 @@ function comment_get_display_page($cid, $node_type) {
+  return $comment->edit();

+++ b/core/modules/entity/entity.module
@@ -453,6 +454,109 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+function entity_edit($entity, $context = 'default') {
...
+ * Implements hook_forms().
...
+function entity_forms($form_id, $args) {

hook_forms() as well as the change to make base_form_ids multiple is unnecessary, since you have a unique and dedicated entry point with ->edit() already.

Instead of drupal_get_form(), you want to use drupal_build_form(), and supply a prepopulated $form_state to it with the base_form_id and any special args predefined. See config_get_form() in #1324618-52: Implement automated config saving for simple settings forms for an example for how to do this.

--
Lastly, I'm not happy about the term "edit" being used here. Add and edit forms are identical with regard to their entity/field API implementation and behavior, so I expect a "form", not "edit".

catch’s picture

Yeah ->form() seems better than edit() to me as well here. I only had a quick read through but I couldn't see much to complain about beyond what's already covered.

plach’s picture

Assigned: Unassigned » plach

Working on this.

plach’s picture

Status: Needs work » Needs review
FileSize
39.95 KB
4.95 KB

Again, tests yet to be fixed.

@sun:

hook_forms() as well as the change to make base_form_ids multiple is unnecessary, since you have a unique and dedicated entry point with ->edit() already.
Instead of drupal_get_form(), you want to use drupal_build_form(), and supply a prepopulated $form_state to it with the base_form_id and any special args predefined.

I tried to use this technique but it turns out that if no function named after the form id is found hook_forms() is invoked, and only the values returned by the implementations are inspected for a base id. Explicitly setting the base id in the form state has no effect in this case.
Moreover reverting the changes to form.inc prevents the form from being altered at three granularity levels: hook_form_entity_form_alter(), hook_form_ENTITY_TYPE_form_alter() and hook_form_BUNDLE_NAME_ENTITY_TYPE_form_alter(). IMO there could be use cases for all of them.

Anyway, I changed entity_edit() to entity_get_form() and EntityInterface::edit() to EntityInterface::form().

Do you guys think I should just fix failing tests, or should I try to go on and try and generalize a bit the actual builders and validate/submit handlers in this issue?

plach’s picture

Sorry, wrong search/replace.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-14.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +sprint

Adding sprint tag to make it easier to follow.

webflo’s picture

Patch looks good.

+++ b/core/modules/node/node.pages.incundefined
@@ -334,7 +333,8 @@ function node_form($form, &$form_state, Node $node) {
-    '#submit' => array('node_form_submit'),
+// TODO
+//     '#submit' => array('node_form_submit'),

@@ -357,13 +357,13 @@ function node_form($form, &$form_state, Node $node) {
-  $form['#validate'][] = 'node_form_validate';
+// TODO
+//   $form['#validate'][] = 'node_form_validate';

Whats about this TODOs? I think this is already done in NodeFormController::submit and NodeFormController::validate

I think the patch is already abstracted enough and we just need to fix the failing tests in Poll module.

plach’s picture

Status: Needs work » Needs review
FileSize
60.55 KB
38.06 KB

The attached patch addressed the remaining TODOs and introduces automatic generation of form actions with the related validate/submit handlers. Moreover global submit handlers are now dismissed in favor of button-level ones: entity prebuilders are introduced to replace those and allow modules to alter the submitted values before the entity object is built from them. Tests not fixed yet, but now it should be possible to.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-18.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

A feedback about the direction taken into the latest patch would be welcome before starting to fix the failing tests.

webchick’s picture

Issue tags: +Entity system

Tagging to get some eyeballs on this.

plach’s picture

This should reduce notices. Again, please confirm the approach is good.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-22.patch, failed testing.

fago’s picture

Ok, here a first review. Honestly, I think it's a bit too early to start working on that monster-area, but it might be a good idea to start cleaning up this asap. So here it is:

+++ b/core/modules/comment/comment.module
@@ -782,6 +784,14 @@ function comment_node_page_additions(Node $node) {
+  return entity_create('comment', $values)->form();

I'm not sure we want that accessor in the entity object itself. We might want to have that, we might not as it bloats the entity with lots of accessors and contribs won't be able to add their accessors for their stuff anyway.

The primary way to make use of an an entity-related API should not be via the entity object, but via other APIs that make use of the entity object. I.e. have entity_form_controller($type)->form() as discussed in #1302378: Use multiple specialized entity controllers?

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,120 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * Returns a rendered edit form for the given entity and context.

I'd agree with sun that we should talk of "entity forms", not edit form. It's for add as well as edit, so it's going to be the main entity form.

hook_forms() as well as the change to make base_form_ids multiple is unnecessary, since you have a unique and dedicated entry point with ->edit() already.

Again, I second that. There is no need for that form api changes. Again, see the entity api module implementation for an example of how you can do that: http://drupalcontrib.org/api/drupal/contributions!entity!entity.module/f...

That said, I think the current form api "entry-point" is quite a mess as we go via a generic entrance, generate specific form-ids for alterations and then map those back in hook_forms() again. We might want to clean that up first.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+  /**
+   * The name of the current context.
+   *
+   * Subclasses may use this value to implement different behaviors depending on
+   * this value.
+   *
+   * @var string
+   */
+  protected $context;

I must say the whole context overall confuses me and the term is already quite overloaded. Anyway, I don't think this or bundle-specific controller classes are needed. We have already a single point for form-customizations: the controller class. Override it, if necessary. I don't think we should bake in other form-customization variants - one is enough.

If necessary the controller class is going to be overridden by the entity-providing module. For modules, we still have form alters..

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+  public final function build(array $form, array &$form_state, EntityInterface $entity) {

Why is that final?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+  protected function actions(array $form, array &$form_state, EntityInterface $entity) {

This mixture between actions() and actionsElement() confuses me. It doesn't seem to be much difference? So is that extra-layer of specifying actions in form-api like structure necessary?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+    if (isset($form['#entity_prebuilders'])) {

We already have http://api.drupal.org/api/drupal/includes!common.inc/function/entity_for... and entity builders. Why do we need pre-builders?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+  public function delete(array $form, array &$form_state) {}

Do we handle the delete case as well? I'd see the need for X-operations then..? I think we might put some effort into operationForms instead, so you can do a "publish" operation which just presents a confirm form rather easily. See the entity api module UI controller for an example.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+  public static final function getEntity($form_state) {

Why do we have static methods here?

+++ b/core/modules/poll/poll.module
@@ -336,6 +343,8 @@ function poll_form(Node $node, &$form_state) {
+  $form['#entity_prebuilders'][] = 'poll_node_form_submit';

That should be a regular entity builder.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php
@@ -0,0 +1,48 @@
+    taxonomy_form_term_submit_build_taxonomy_term($form, $form_state);

We'll need to move all the form related helpers into the controller class. That's what they are there for. That is, we'll need to move the submit-build helper as well as the field api validation helper to the controller.

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -813,7 +790,7 @@ function taxonomy_form_term_validate($form, &$form_state) {
-  $term = taxonomy_form_term_submit_build_taxonomy_term($form, $form_state);
+  $term = EntityFormController::getEntity($form_state);

That's hard-coding "EntityFormController" although a different controller class is used. Instead, I think it's the job of the form controller to add suiting validation and submit callbacks so they get called appropriately. Then, once called there is no need for static helpers. Again, take a look at the entity api module's ui controller to get some ideas.

+++ b/core/modules/user/lib/Drupal/user/ProfileFormController.php
@@ -0,0 +1,75 @@
+class ProfileFormController extends EntityFormController {

Should be UserAccount or UserFormController. The term "profile" is deprecated for user.module and a left-over we should not continue with.

+++ b/core/modules/user/lib/Drupal/user/ProfileFormController.php
@@ -0,0 +1,75 @@
+    // @todo Legacy support. Modules are encouraged to access the entity using
+    //   $form_state. Remove in Drupal 8.
+    $form['#user'] = $account;
+
+    user_account_form($form, $form_state);
+
+    return parent::form($form, $form_state, $account);

This should define the user_account_form() itself. Then, imo the registration form should just make use of the same controller and massage the form as needed afterwards. For additional form context, we already have form state. I.e. just add something like $form_state['register'] = TRUE.

plach’s picture

Just a quick reply to the major points, I'll have a full review tonight.

Ok, here a first review. Honestly, I think it's a bit too early to start working on that monster-area, but it might be a good idea to start cleaning up this asap.
[...]
We'll need to move all the form related helpers into the controller class. That's what they are there for. That is, we'll need to move the submit-build helper as well as the field api validation helper to the controller.

The aim of this patch is providing an initial infrastructure to ensure D8MI can proceed on the entity front, certainly not solving every problem in this area ;)
I'm trying to keep the patch size minimal, hence I didn't move the various validate/submit handler into the form controllers. I totally agree this should be done, along with other refactorings to try and reuse the more code possible. However this is out of scope for this patch IMO.

Again, I second that. There is no need for that form api changes. Again, see the entity api module implementation for an example of how you can do that: http://drupalcontrib.org/api/drupal/contributions!entity!entity.module/f...

As I said I'd like the impact of this patch to be minimal, so I'd be more than happy to avoid hacking form.inc. However I tried to implement @sun's suggestion with no luck: I'll review the code above and try to understand what I'm missing.

That said, I think the current form api "entry-point" is quite a mess as we go via a generic entrance, generate specific form-ids for alterations and then map those back in hook_forms() again. We might want to clean that up first.

Again, this is how things work now in form.inc: I don't know how to streamline this process and keep the ability of choosing the granularity of the form alteration (global, per-entity, per-bundle), without going the way proposed in the patches here or without deeper refactorings in the Form API.

I must say the whole context overall confuses me and the term is already quite overloaded. Anyway, I don't think this or bundle-specific controller classes are needed. We have already a single point for form-customizations: the controller class. Override it, if necessary. I don't think we should bake in other form-customization variants - one is enough.

The point of contexts (and we can certainly pick a different name, not sure which is more appropriate though) is giving the ability to provide multiple different entity forms: implementing this behavior through alterations, instead of being able to declare a different controller for each case, looks as a fragile/limiting approach to me. Also the point of per-bundle controllers is letting a module like Poll have its own specialized controller. I'd like to confine alterations to the extending modules, modules providing the entity (or the bundle) should be able to rely on a dedicated controller if they wish to.

That's hard-coding "EntityFormController" although a different controller class is used. Instead, I think it's the job of the form controller to add suiting validation and submit callbacks so they get called appropriately. Then, once called there is no need for static helpers. Again, take a look at the entity api module's ui controller to get some ideas.

Sure, this already somehow prepared in the code but, as I was saying above, not completely exploited yet. However I'll have a closer look to the suggested code before the next iteration. Sorry for not doing that before.

plach’s picture

In addition to #25, still answering to #24:

I'm not sure we want that accessor in the entity object itself. We might want to have that, we might not as it bloats the entity with lots of accessors and contribs won't be able to add their accessors for their stuff anyway.

The 'consistency with contrib' argument is valid, altough I guess also having shortcuts for the most common controllers might make sense. No strong opinion here, just tell me which one you prefer.

The primary way to make use of an an entity-related API should not be via the entity object, but via other APIs that make use of the entity object.

Sure, I guess this is not particularly evident because of the messy Form API workflow, but the entity_form() function does exactly that, entity_get_form() is just the Form API counterpart. However I'll try to stick more close to the naming conventions described in #1302378: Use multiple specialized entity controllers.

I'd agree with sun that we should talk of "entity forms", not edit form.

I thought I already replaced all the occurences :)

Why is that final?
[...]
This mixture between actions() and actionsElement() confuses me. It doesn't seem to be much difference? So is that extra-layer of specifying actions in form-api like structure necessary?

Well, the idea behind final methods was to be able to bake-in some fixed logic and to rely on it later. EntityFormController::form() and EntityFormController::actions() are just the "overridable" counterparts of EntityFormController::build() and EntityFormController::actionsElement().

Having a separate call for EntityFormController::actionsElement()/EntityFormController::actions() lets us provide some sane default beaviors with their validate/submit handlers already in place. In my mind most entity-specific overriding of the actions won't be needed, while certainly some customization of the form itself will. It's pure form api, btw, not form-api like stuff: it's saying just give me the actions element and (reliably) massaging it a bit afterwards.

Why do we need pre-builders?

Honestly I ain't sure about those either: on one hand a piece of code needing to run before entity building even starts woud work more reliably if it could exploit pre-builders, otherwise it might be executed after the entity building phase has already begun, depending on the form alteration order. An example here would be node_field_language_form_submit. OTOH we have this kind of problem all the time, not sure whether this particular case deserves a dedicated solution.

Do we handle the delete case as well?

Well, most core entity forms currently do in a way or another. This is an attempt to just keep things working. However, I will have a look to operationForms and check whether they are what we need here.

Why do we have static methods here?

I wanted to encapsulate access to the entity object even without requiring an EFC instance, however probably making the controller more stateful and store the entity object directly into it might make more sense, right? For external callbacks we would need something like the following, anyway:

EntityFormController::getFormInstance($form_state)->getEntity();
That should be a regular entity builder.

Yes, you are right. It could even be called from PollFromController::submit(), however (perhaps more reliably).

Should be UserAccount or UserFormController. The term "profile" is deprecated for user.module and a left-over we should not continue with.

Noted :)

Then, imo the registration form should just make use of the same controller and massage the form as needed afterwards. For additional form context, we already have form state. I.e. just add something like $form_state['register'] = TRUE.

I'm sorry but I don't agree with this statement: why do two conceptually different forms (aside from the fact that both deal with the user entity) need to share the same code and differentiate through a bunch of ifs when we have a cleaner way to do that which is inheritance? Why don't we just make both inherit from a base class and live two distinct lives? I bet this would be easier to maintain besides being cleaner to look at.


To sum up, my personal goal for this patch is providing the first step towards Entity Form Architectural Nirvana™. I don't expect everything to look perfect since the very first iteration. The patch is already pretty big without fixing tests, I'm afraid it'll get huge by time we manage to fix all the things we all would like to :)

plach’s picture

@sun:

I tried once again to use drupal_build_form() and provide a base form id directly in the build info with no luck. The code in drupal_retrieve_form() totally ignores it without an implementation of hook_forms(). I have no idea of how you made the config form work in the patch linked above :(

plach’s picture

Status: Needs work » Needs review
FileSize
60.6 KB

Just rerolled the last patch for now. Trying to implement @sun's and @fago's suggestion.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-28.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
59.47 KB
5.52 KB

I tried to make @sun's suggestion work, but touching the Form API is still needed to retain BC and make forms alterable at least at bundle and entity type level. However overall the form building process looks quite cleaner now and the Form API changes are definitely smaller.

All the rest is still to do.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-30.patch, failed testing.

Jose Reyero’s picture

Though I haven't really got in dept into this patch, it seems to me that a FormController is something (an interface) we could better have in form API, then extend it for Entities.

plach’s picture

@Jose:

I think something along those lines is being proposed in #597280: Introduce form registry to fix various problems and increase security. As I was writing above, for this patch I would be happy to just introduce a basic common denominator for entity forms. Refinements will be welcome in the follow-ups.

tstoeckler’s picture

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+  // If no controller is specified default to the base implementation.
+  if (!empty($info) && empty($info['form controller class'])) {
+    $class = 'Drupal\entity\EntityFormController';
+  }
+
+  // Check whether a bundle-specific form controller exists.
+  if (!empty($bundle) && !empty($info['bundle form controller class'][$bundle][$context])) {
+    $class = $info['bundle form controller class'][$bundle][$context];
+  }
+  // Fall back to the per-entity-type form controller.
+  elseif (!empty($info['form controller class'][$context])) {
+    $class = $info['form controller class'][$context];
+  }
+  // If a non-existing context has been specified stop.
+  else {
+    throw new EntityMalformedException("Missing form controller for '$entity_type' bundle '$bundle' in context '$context'");
+  }

Seems like, because of the if/elseif/else below, the first if is never actually used. I might be missing something.

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+  $form_id = $entity_type;
+  if ($bundle != $entity_type) {
+    $form_id = $bundle . '_' . $form_id;
+  }
+  if ($context != 'default') {
+    $form_id = $form_id . '_' . $context;
+  }
+  return $form_id . '_form';

Is there reasoning the preprend $bundle but append $context? So we have "page_node_form", like we currently do? I think decreasing granularity in general makes more sense, e.g. "{$entity_type}_{$bundle}_{$context}_form"

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ *   The entity being edited or the entity type to be created.

This sounds like the whole entity type is being created. I get what it's trying to say, but I think it could be clarified.

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+  $form_state['build_info']['base_form_id'] = $entity->entityType() . '_form';

I'm not really up-to-date on the Form API specifics. Can we have multiple "base_form_id"s? I could see both "entity_form" and "node_form" (i.e. "{$entity_type}_form") making sense. Might be a followup-issue.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+    $submit = array($this, 'submit');
...
+      array_unshift($element[$action]['#submit'], $submit);

This is again bikeshedding, but is there a reason your putting $submit into a dedicated variable?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+      'submit' => array(

Seems weird to call this 'submit' instead of 'save'. (This is the element key in the form array.)

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+    if (isset($form['#entity_prebuilders'])) {

This is a strange name. Maybe "#entity_prebuild_handlers" or "..._callbacks". Or simply "#prebuild", that would be inline with "#validate" and "#submit".

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,285 @@
+    // @todo Introduce a new language type to use as the default language.

I don't understand this comment. Can you elaborate?

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -857,8 +834,7 @@ function taxonomy_form_term_submit($form, &$form_state) {
-  $term = $form_state['term'];
...
+  $term = EntityFormController::getEntity($form_state);

Here and elsewhere: Maybe we should simply document $form_state['entity'] for entity forms (or $form_state[$entity_type], though I prefer the former), and get rid of the getEntity/setEntity accessors. We really do that with all kinds of stuff in $form_state, ($form_state['values'] probably being the most prominent example), so I really can't come up with a valid reason against this.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -315,10 +321,10 @@ function taxonomy_menu() {
   $items['taxonomy/term/%taxonomy_term/edit'] = array(
...
-    'page callback' => 'drupal_get_form',
+    'page callback' => 'entity_get_form',

Seems like the other entities are missing this change. Is there a reason for that? I mean tests pass, so I guess there is...

+++ b/core/modules/user/lib/Drupal/user/ProfileFormController.php
@@ -0,0 +1,75 @@
+  public function save(array $form, array &$form_state) {
+    $account = EntityFormController::getEntity($form_state);

Seems like maybe we could just be nice in the parent submit function and pass $entity directly to the save function?!

+++ b/core/modules/user/lib/Drupal/user/RegisterFormController.php
@@ -0,0 +1,62 @@
+class RegisterFormController extends EntityFormController {
...
+  protected function actions(array $form, array &$form_state, EntityInterface $account) {
+    $element = parent::actions($form, $form_state, $account);
+    $element['submit']['#value'] = t('Create new account');
+    return $element;
+  }

I might be wrong (I didn't try this patch out), but doesn't put a "Delete" button on the user registration form? That would be strange...

On the final methods: I think splitting actions() and actionsElement() makes sense for the reason you noted, but I don't think we need to make actionsElement() (and all of the other functions) final. We should document, that in the 90%-case all you have to do is override actions() (or not even that, but that's not the point), but if you want to go crazy, you can override actionsElement() and do whatever you want.

Regardless of whether we have a generic FormController or FormControllerInterface we should have an EntityFormControllerInterface that EntityFormController implements.

Regarding $context: I think $variant would make more sense. I also contemplated $form_view_mode (or just $view_mode?). I think we should avoid $context, as it's already has roundabout 20 meaning within Drupal.

plach’s picture

Assigned: plach » nod_
Status: Needs work » Needs review
FileSize
60.57 KB

@tstoeckler:

Thanks for the review (I saw it right now), you are raising some valid points! I'll try to address them along with fago's ones after fixing the tests (OpenID ones should be ok now).

Bot?

plach’s picture

Assigned: nod_ » plach

wtf?

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-35.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
67.54 KB

This should fix some more tests/bugs.

plach’s picture

Great! Now that we are sure that the current approach is not irremediably broken I'll try to address the pending issues from the reviews above.

plach’s picture

I started implementing @fago's suggestions. The attached patch removes most static methods from the EFC and improves entity encapsulation. Posting it just to see if something broke, but it's only intermediate work.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-40.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
70.32 KB

This should work a bit better :)

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-42.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
72.08 KB

Again, this should work better. Working on the rest of the reviews.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-44.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
72.17 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-46.patch, failed testing.

tstoeckler’s picture

I read through this again, and am really liking this a lot. I found a bunch of more things to complain about, this time I went really nitpicky with docs and such, though. So nothing major.

+++ b/core/modules/comment/comment.module
@@ -786,6 +790,14 @@ function comment_node_page_additions(Node $node) {
+  $values = array('nid' => $node->nid, 'pid' => $pid, 'node_type' => 'comment_node_' . $node->type);

Let's break that into multiple lines.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -0,0 +1,76 @@
+class CommentFormController extends EntityFormController {
...
+  protected function actions(array $form, array &$form_state) {
...
+    $element['preview'] = array(

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+  /**
+   * Returns an array of supported actions for the current entity form.
+   */
+  protected function actions(array $form, array &$form_state) {

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -0,0 +1,84 @@
+class NodeFormController extends EntityFormController {
...
+  protected function actions(array $form, array &$form_state) {
...
+    $element['preview'] = array(

Can we add a @todo to EntityFormController::actions about adding a 'Preview' action, since a bunch of entities in core already duplicate that?

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -0,0 +1,76 @@
+    $form = comment_form($form, $form_state, $this->entity);
...
+    comment_form_validate($form, $form_state);
...
+    comment_submit($this->entity);
...
+    comment_form_submit($form, $form_state);

I don't think that needs to be done in this issue, but I think we should add @todo's to remove the procedural functions alltogether.

+++ b/core/modules/entity/entity.api.php
@@ -27,6 +27,12 @@
+ *   - form controller class: An array of form controller class names keyed by
+ *     context name. Each class handle the edit form for the entity being
+ *     defined, alongside with its handlers, for the related context. The
+ *     context is also passed to the class costructor hence if only small tweaks
+ *     are needed to adapt the edit form to the various contexts a unique class
+ *     may be provided.

There were a couple and typos and things I found weird in this paragraph, so I took the liberty of directly providing an improved version, instead of a bunch of detailed, but minor reviews. It's (quite) a bit more verbose, but hey...:

An associative array where the keys are the names of the different form contexts and the values are the names of the form controller classes. To facilitate the case where an entity form varies only slightly between different contexts, the name of the context is passed to the constructor of the form controller class. In that way, one class can be used for multiple contexts.

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * Returns a form controller for the given context.

I'm not quite sure where, but I think the word "entity" should be in there somewhere. :-)

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * Since there might be different contexts in which an entity or parts of it are
+ * edited, multiple form controllers suitable to the different contexts may be
+ * defined. If no valid controller is found for the given context the default
+ * one will be used.

I think the "parts of it" part can be omitted. I'm not sure what you mean by it, but it doesn't seem essential in this function header.

Since this basically describes, what is documented above in hook_entity_info(), maybe add a @see?

In the second sentence the "valid" can be ommitted I think (since we really perform no validation).

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+  if (!empty($bundle) && !empty($info['bundle form controller class'][$bundle][$context])) {

This key is not documented in hook_entity_info(). Also, shouldn't it be inside the 'bundles' array? I.e. rather $entity_info['bundles'][$bundle]['form controller class'][$context]? If I'm not mistaken, that would even allow a bit more flexibility in terms of bundle-specific overrides.

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * Helper function. Returns the form id for the given entity.

Omit the "Helper function." part.

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * Returns a rendered edit form for the given entity and context.

Now it's getting really nitpicky, but maybe replace "the" with "a"?!

+++ b/core/modules/entity/entity.module
@@ -447,6 +450,90 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * @param EntityInterface|string $entity
+ *   The entity being edited or the entity type to be created.

I generally really dislike these sorts of double variables. In theory, we could call this "entity_type_get_form($entity_type, $bundle, $context)" and then have a separate "entity_get_form($entity, $context)" call this with $entity->entityType() and $entity->bundle(). The way it is now is kind of backwards IMO, but on the other hand I'm not sure whether two functions is actually worth it.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+class EntityFormController {
+
+  /**

I always omit that blank line, is there a standard for this? (I'm genuinely asking, it might very well be that this is the correct way.)

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+    elseif (isset($element['delete'])) {
+      // Move the delete action as last one, unless weights are explictly
+      // provided.
+      $delete = $element['delete'];
+      unset($element['delete']);
+      $element['delete'] = $delete;
+    }
...
+        '#weight' => ++$count * 5,

Seems like this could be more easily achieved by simply adding a default weight of 5 or something to the 'Delete' action. That would collide with the setting of the weights below, but I don't really get, why we do that anyway.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+    // @todo Exploit the Property API to process the submitted  entity property

double space between "submitted" and "entity"

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+   * Gives a chance to manipulate submitted data before building the entity.

Maybe simply "Executes entity prebuilder callbacks." (Or whatever the proper name for these things is.)

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+   * Submit handler for the 'save' action.
...
+   * Submit handler for the 'delete' action.

Is there some sort of standard for this? With foo_bar_form_submit() we do "Form submission handler for foo_bar_form()."

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+   * @param $form
+   *   An associative array containing the structure of the form.
+   * @param $form_state
+   *   A reference to a keyed array containing the current state of the form.
...
+   * @param array $form
+   * @param array $form_state

Copy-paste please! :-)

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+    $language = $this->entity->language();
...
+  public function getEntity() {
+    return $this->entity;
+  }

Hmm... If we want people to be able to subclass this easily, shouldn't we use $this->getEntity() intenally as well. Otherwise if people override getEntity(), the above won't work.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,286 @@
+  public static final function getFormInstance($form_state) {
...
+  protected final function setFormInstance(&$form_state) {

IMO it's super weird to have one of the static and the other not. for setFormInstance() we could simply pass $this explicitly when calling.

+++ b/core/modules/system/system.api.php
@@ -1355,9 +1355,10 @@ function hook_form_BASE_FORM_ID_alter(&$form, &$form_state, $form_id) {
+ *   - base form ids: An array containing a list of base form ids to be used to

ids -> IDs

+++ b/core/modules/system/system.api.php
@@ -1355,9 +1355,10 @@ function hook_form_BASE_FORM_ID_alter(&$form, &$form_state, $form_id) {
+ *     provided the callback function name will be used.

comma after "provided"

+++ b/core/modules/system/tests/modules/taxonomy_test/taxonomy_test.module
@@ -59,16 +59,14 @@ function taxonomy_test_taxonomy_term_delete(Term $term) {
  * Implements hook_form_alter().
...
-function taxonomy_test_form_alter(&$form, $form_state, $form_id) {
...
+function taxonomy_test_form_taxonomy_term_form_alter(&$form, $form_state, $form_id) {

Should now be "Implements hook_form_FORM_ID_alter()."

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.php
@@ -0,0 +1,55 @@
+ * Base for controller for taxonomy term edit forms.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -0,0 +1,62 @@
+ * Base for controller for vocabulary edit forms.

for -> form

Also neither of these are overriden or subclassed anywhere, so I don't think it's reasonable to call them "base form controllers"

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -0,0 +1,62 @@
+    $form = taxonomy_form_vocabulary($form, $form_state, $this->entity);

For taxonomy_term you renamed taxonomy_form_term() to taxonomy_term_form() (which totally makes sense). Don't know if we want to do likewise, here.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -0,0 +1,62 @@
+    // If we are displaying the delete confirmation skip the regular actions.
+    return empty($form_state['confirm_delete']) ? parent::actions($form, $form_state) : array();
...
+    if ($form_state['triggering_element']['#value'] == t('Delete')) {
+      // Rebuild the form to confirm vocabulary deletion.
+      $form_state['rebuild'] = TRUE;
+      $form_state['confirm_delete'] = TRUE;
+    }

I know this wasn't introduced here, but can we please add a @todo to not call taxonomy_vocabulary_confirm_delete() from within taxonomy_form_vocabulary() ?

tim.plunkett’s picture

I can't find it at the moment, but a blank line after the class declaration is correct:

class foo {

  function bar() {
  }

}
plach’s picture

Status: Needs work » Needs review
FileSize
22.59 KB
72.62 KB

The attached patch implements the suggestions from #24, #34, and #48 with some exceptions. In many cases the reason behind not implementing a particular suggestion is keeping the patch size minimal. I'd wish we could commit something similar to what we have here and agree on some follow-ups to clean things up in smaller chunks.

It's too late now to provide a detailed description and rationale. I'll do it tomorrow along with a list of the proposed follow-ups.

Meanwhile let's see if the bot is happy. Attached you can find also an interdiff with #46.

plach’s picture

Here are some detailed answers:

@fago:

I'm not sure we want that accessor in the entity object itself. We might want to have that, we might not as it bloats the entity with lots of accessors and contribs won't be able to add their accessors for their stuff anyway.

Removed EntityInterface::form() and left entity_get_form() as the main way to retrieve an entity form.

hook_forms() as well as the change to make base_form_ids multiple is unnecessary, since you have a unique and dedicated entry point with ->edit() already.

As suggested by @sun in IRC and in his sandbox, the attached patch uses drupal_build_form() and removes the implementation of hook_forms() with some minimal tweak to the Form API. The main reason behind those is retaining BC and not being forced to rename all the hook_form_FORM_ID_alter() and hook_form_BASE_id_alter() implementations all over the place. If also this solution is deemed too invasive I'd try to move the call to drupal_build_form() into EntityFormController::build() and return a renderable (form) array.

The current approach retains the ability to alter the form at bundle level and entity-type level. We cannot alter any entity form anymore, but this behavior can be "emulated" by implementing hook_form_alter() and checking that EntityFormController::getFormInstance() returns a valid form controller instance (a non-false value).

I must say the whole context overall confuses me and the term is already quite overloaded. Anyway, I don't think this or bundle-specific controller classes are needed.

After studying the Entity API's UI controller more carefully, I noticed some similarity between the concept of context here and the operation forms there. For this reason I didn't try to remove it for now, as I'd like to discuss/explore this similarity some more. However per-bundle controllers are gone, I'd open a dedicated follow-up for them if anyone here thinks they shouldn't be just dropped.

Why is that final? [...]
This mixture between actions() and actionsElement() confuses me. [...]

I provided an answer to this point in #26. Any feedback?

Why do we need pre-builders?

Prebuilders are gone now.

Why do we have static methods here?

Statics are gone. The entity object is now stored directly into the controller as a protected field. This improves encapsulation and should give us a more reliable way to access the entity from form the various callbacks, instead of the bunch of inconsistent ways we have now.

That's hard-coding "EntityFormController" although a different controller class is used. Instead, I think it's the job of the form controller to add suiting validation and submit callbacks so they get called appropriately. Then, once called there is no need for static helpers. [...]

This now happens only for the form controller instance static accessors which are final as there should be no need to override them. Better enforce some consistency here. They are needed in any "external" alter/process/validate/submit callback to access the entity.

This should define the user_account_form() itself. [...]

I already replied to this statement in #26. Quoting myself:

I'm sorry but I don't agree with this statement: why do two conceptually different forms (aside from the fact that both deal with the user entity) need to share the same code and differentiate through a bunch of ifs when we have a cleaner way to do that which is inheritance? Why don't we just make both inherit from a base class and live two distinct lives? I bet this would be easier to maintain besides being cleaner to look at.

What do you think?


@tstoeckler:

Is there reasoning the preprend $bundle but append $context? So we have "page_node_form", like we currently do? I think decreasing granularity in general makes more sense, e.g. "{$entity_type}_{$bundle}_{$context}_form"

The main reason here is keeping BC, for instance having user_profile_form, but your proposal makes sense. We might want to address it in a follow-up.

I'm not really up-to-date on the Form API specifics. Can we have multiple "base_form_id"s? I could see both "entity_form" and "node_form" (i.e. "{$entity_type}_form") making sense. Might be a followup-issue.

Well, this was exactly the approach I originally chose and that both @sun and @fago refused as it implied some changes to the Form API.

This is again bikeshedding, but is there a reason your putting $submit into a dedicated variable?

Just readability, the array looked less clear to me.

Seems weird to call this 'submit' instead of 'save'. (This is the element key in the form array.)

Again BC: we would need to change all the places accessing 'submit' key. A follow-up would be appropriate here.

Here and elsewhere: Maybe we should simply document $form_state['entity'] for entity forms [...]

Not in my mind: D8 is moving to OOP, which provides encapsulation, which in turn provides more reliability/testability/ease in refactoring things and all this stuff you know better than me. Ax a general rulem, I think we should exploit this capabilities whenever they make sense and keep structured arrays mostly for dynamic data definitions, such forms and info arrays.

I might be wrong (I didn't try this patch out), but doesn't put a "Delete" button on the user registration form? That would be strange...

No :) Look at EntityFormController::actionsElement().

On the final methods: I think splitting actions() and actionsElement() makes sense for the reason you noted, but I don't think we need to make actionsElement() (and all of the other functions) final.

Not sure about this: we can still rely on alterations to change anything on the form we wish to. IMO it's important that we somehow codify what is supposed to be changed and what might be dangerous to. A final method is a clear indication, a PHP doc might be ignored or missed.

Regardless of whether we have a generic FormController or FormControllerInterface we should have an EntityFormControllerInterface that EntityFormController implements.

This depends on whether we want to make the entity form controller implementation swappable: I don't see many use cases requiring to entirely replace such a complex system. This might use a bit more discussion.

Let's break that into multiple lines.

Why? In most of the places I saw it's just one line.

I generally really dislike these sorts of double variables.

Me too :) The string-only usage is gone.

Seems like this could be more easily achieved by simply adding a default weight of 5 or something to the 'Delete' action. That would collide with the setting of the weights below, but I don't really get, why we do that anyway.

Again it's mostly to retain pseudo-BC: it seems a common pattern to use a "5 step" weight among action widgets.

Hmm... If we want people to be able to subclass this easily, shouldn't we use $this->getEntity() intenally as well. Otherwise if people override getEntity(), the above won't work.

I'd tend to agree, put we should make the $entity field private, then. It seems the general trend for DrOOPal is using protected variables, so I picked this visibility mainly for consistency. Any other thoughts about this?

Don't know if we want to do likewise, here.

I tried but some tests broke and fixing them implied unnecessary changes that would increase the patch size.


Aside from the ones hinted in the answers above, here is a list of follow-ups I'd open after committing (something like) the attached patch:

  • Move all the callbacks involved in the entity form workflow and defined by the entity-providing modules in their form controllers. One issue per entity type.
  • Refactor the result to move as much code as possile to the base form controller in order to increase code reusage, descrease the codebase size and so on.
  • Implement the various operation forms as contexts (or whatever we will end up calling them), if we agree they are related.
  • Rename all the occurences of the Profile term in the User module and friends.

What do you think?

Status: Needs review » Needs work
Issue tags: -Entity system, -D8MI, -sprint, -language-content

The last submitted patch, entity_forms-1499596-51.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +D8MI, +sprint, +language-content

#51: entity_forms-1499596-51.patch queued for re-testing.

chx’s picture

plach asked me to look at the form.inc change, it's a good one we should do more of that in #1454730: Allow OO methods as FAPI / render API #callbacks (which smells major task to me now).

plach’s picture

A small improvement: the attached patch makes the entity form state reusable and slightly improves the entity form API.

Reviews welcome :)

fago’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -786,6 +790,15 @@ function comment_node_page_additions(Node $node) {
+ * Returns a rendered form to comment the given node.
+ */
+function comment_edit($node, $pid = NULL) {
+  $values = array('nid' => $node->nid, 'pid' => $pid, 'node_type' => 'comment_node_' . $node->type);
+  $comment = entity_create('comment', $values);
+  return entity_get_form($comment);

This actually doesn't show a comment edit form, does it? It's a comment add form.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -0,0 +1,76 @@
+ * Base for controller for comment edit forms.
+ */

Just comment form - no edit?

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -0,0 +1,76 @@
+  public function validate(array $form, array &$form_state) {
+    parent::validate($form, $form_state);
+    comment_form_validate($form, $form_state);
+  }

I know you try to avoid moving code around, but implementing a class for just forwarding the call is kind of strange. Still, we could clean that up in follow-ups.

+++ b/core/modules/node/node.pages.inc
@@ -567,19 +524,7 @@ function node_field_language_form_submit($form, &$form_state) {
 function node_form_submit_build_node($form, &$form_state) {
-  // @todo Legacy support for modules that extend the node form with form-level
-  //   submit handlers that adjust $form_state['values'] prior to those values
-  //   being used to update the entity. Module authors are encouraged to instead
-  //   adjust the node directly within a hook_node_submit() implementation. For
-  //   Drupal 8, evaluate whether the pattern of triggering form-level submit
-  //   handlers during button-level submit processing is worth supporting
-  //   properly, and if so, add a Form API function for doing so.
-  unset($form_state['submit_handlers']);
-  form_execute_handlers('submit', $form, $form_state);
-
-  $node = $form_state['node'];
-  entity_form_submit_build_entity('node', $node, $form, $form_state);
-
+  $node = EntityFormController::getFormInstance($form_state)->getEntity();

Instead of keeping entity-type specific submit_build_entity functions, I think that's something that needs to go into the controller. Then, the controller can make use of it for invoking submit handlers that already get the entity passed.

+++ b/core/modules/user/user.module
@@ -3538,54 +3548,21 @@ function user_register_form($form, &$form_state) {
  * Submit handler for the user registration form.
  *
- * This function is shared by the installation form and the normal registration form,
- * which is why it can't be in the user.pages.inc file.
- *
- * @see user_register_form()
+ * This function is shared by the installation form and the normal registration
+ * form, which is why it can't be in the user.pages.inc file.
  */
 function user_register_submit($form, &$form_state) {
+  $account = EntityFormController::getFormInstance($form_state)->getEntity();
+  $pass = $account->pass;
   $admin = $form_state['values']['administer_users'];
-
-  if (!variable_get('user_email_verification', TRUE) || $admin) {
-    $pass = $form_state['values']['pass'];
-  }
-  else {
-    $pass = user_password();
-  }
   $notify = !empty($form_state['values']['notify']);
 
-  // Remove unneeded values.
-  form_state_values_clean($form_state);
-
-  $form_state['values']['pass'] = $pass;
-  $form_state['values']['init'] = $form_state['values']['mail'];
-
-  $account = $form['#user'];
-
-  entity_form_submit_build_entity('user', $account, $form, $form_state);

Actually, that's wrong. It's no submit handler any more and that is where just forwarding calls to existing submit handler starts getting really confusing.

Also, that code does away with the submit builder? (Again, if the builder would run on the controller level and this would just get the entity passed would simplify things).

+++ b/core/modules/user/lib/Drupal/user/RegisterFormController.php
@@ -0,0 +1,62 @@
+ * @see Drupal\entity\EntityFormController::submit()
+ */
+ public function submit(array $form, array &$form_state) {
+ $admin = $form_state['values']['administer_users'];
...
+ * @see Drupal\entity\EntityFormController::submit()

This patch introduces a submit-phase for entity forms, but that duplicates the already existing submit-builders. That's supposed to be the same thing, e.g. see node_form_submit_build_node() which invokes node_submit().

+ * Builds an entity edit form.

Again, edit terminology.

@validation/submit handlers:
I could not see where those a registered for the main form, so I guess entity-forms would just fallback to form_id_VALIDATE callbacks? That would be very problematic if the control flow for invoking validate/submit handlers differs depending on whether a button adds the controller validate handler or not. The controller valdiate should be the default and the only way to invoke entity-specific form validation.

@entity-controller & instances:
I don't think it's a good idea to introduce that state to the controller, first off it doesn't fit to the concept of controllers so it would make it something else. I think it would be better to keep the entity in $form_state only + add the controller instance in use as well. Then, access to the entity can still go via the controller, ala $form_state['controller']->getEntity().

For submit-building I think we want to have a way to build() and get the entity, ala $form_state['controller']->buildEntity() - what builds the entity based upon submitted values. Then there should be $form_state['controller']->submit() what builds the entity + updates the entity in form state. That should be default submit handler and works fine with form-rebuilds, while buildEntity() can be used to get the updated entity object for validation or such (to be figured out in follow-ups).

+1 for supporting OO #callbacks

I'm sorry but I don't agree with this statement: why do two conceptually different forms (aside from the fact that both deal with the user entity) need to share the same code and differentiate through a bunch of ifs when we have a cleaner way to do that which is inheritance? Why don't we just make both inherit from a base class and live two distinct lives? I bet this would be easier to maintain besides being cleaner to look at.

I agree that it makes sense to leverage inheritance for build and controlling that form. However, still I'm not convinced we need to invent that $context system as it's a system for explicitly supporting multiple variations of entity forms.
Imo, its' important that we have a clear and single way to get and adapt the entity form. But that any variations of that form need not be registered in the system, that should be possible by implementing my own form + use the entity form controller + add in my customizations or I use my own version of the form controller to add in my customizations, i.e. I extend the controller, instantiate it myself and use it to control the form.
That said, form alterations of entity forms should happen independently from the real form-id of the form, what could be seen as a problem of general form re-usability and deserves its own issue as well.

Thus, imo registering multiple variations of entity forms should be up to a general form registry, as I don't see the need to re-use multiple variants of entity form specifically.
Note that operations forms (delete, clone, export, ..) are not variants of the main form, they are actually totally different forms. So we should have different mechanism for them, although it might make sense to leverage the same controller? But again, that can be a follow-up.

Gábor Hojtsy’s picture

Priority: Normal » Major

Essential for translatable content.

plach’s picture

Status: Needs review » Needs work
FileSize
75.99 KB
17.94 KB

Actually, that's wrong. It's no submit handler any more and that is where just forwarding calls to existing submit handler starts getting really confusing.

I know, but I'm pretty much sure you'd have more chances to be confused with a 200k patch :)
IMHO having one issue for each core entity to clean up those things and factor up common code is going to be easier to achieve than doing all the hard work here. However by proxying those calls we are ensuring that the cleaned-up code is likely to work without problems.

Instead of keeping entity-type specific submit_build_entity functions, I think that's something that needs to go into the controller. Then, the controller can make use of it for invoking submit handlers that already get the entity passed.
[...]
Also, that code does away with the submit builder? (Again, if the builder would run on the controller level and this would just get the entity passed would simplify things).
[...]
This patch introduces a submit-phase for entity forms, but that duplicates the already existing submit-builders. That's supposed to be the same thing, e.g. see node_form_submit_build_node() which invokes node_submit().

I ain't sure I understand your concerns here, so I'm summarizing the current behavior to ensure we have a common ground for discussion:

  • during the build phase the controller retrieves the form array and the actions;
  • actions are massaged to ensure there is a submit handler for each one rebuilding the entity with the submitted values before any other processing happens;
  • only the save action gets the validation handler because there is no need to validate the submitted values when deleting the entity (I find very tedious that I'm not allowed to delete a node with an empty title);

This is actually splitting the submit-build phase, which may be needed by every action, and the actual submission logic, which is confined in the 'save' submission handler. The base 'submit' handler is not duplicating the submit-build phase, it's just moving it into the controller, which is what you seem to suggest above. This is why I'm a bit confused.

Moreover I ain't sure I get what's the problem below:

I could not see where those a registered for the main form, so I guess entity-forms would just fallback to form_id_VALIDATE callbacks? That would be very problematic if the control flow for invoking validate/submit handlers differs depending on whether a button adds the controller validate handler or not. The controller valdiate should be the default and the only way to invoke entity-specific form validation.

Currently the validation handler is added by the controller in the overridable EntityFormController::actions() method. Are you saying that it should be treated as the submit handler and always added to the 'save' button in the following non-overridable action massaging? If so I'd tend to agree.

I don't think it's a good idea to introduce that state to the controller, first off it doesn't fit to the concept of controllers so it would make it something else. [...]

Agreed. I grudgingly removed the entity encapsulation as your argument above about consistency between controllers makes totally sense. I'm glad you're fine with keeping the getEntity() method as I really want to introduce a unique and consistent way to access the entity object. To do that I had to add the form state as a wrapped field, but I guess this makes more sense and does not violate the definition of controller.

For submit-building I think we want to have a way to build() and get the entity, ala $form_state['controller']->buildEntity() [...]

The attached patch introduces a reusable buildEntity() method that for now is invoked only in the submit-build phase, but that could be theorically used also in validation handlers. Freshness is ensured by a process callback that updates the wrapped form state after cache retrieval and before handlers are invoked.

I agree that it makes sense to leverage inheritance for build and controlling that form. However, still I'm not convinced we need to invent that $context system as it's a system for explicitly supporting multiple variations of entity forms. [...]
Thus, imo registering multiple variations of entity forms should be up to a general form registry, as I don't see the need to re-use multiple variants of entity form specifically.
Note that operations forms (delete, clone, export, ..) are not variants of the main form [...]

Let me explain my point a little more: after looking better at the current Entity API code, I started thinking that what we are likely to need are not micro-variations of the default entity form, but actually a different form for each operation to be performed on the entity: add/edit (default action), clone, export, delete and so on...

What I'm proposing is keeping the context system here and renaming it to use a "operation" terminology so that we can have a common logic to register and instantiate a form controller for each needed action. Currently only the user entity type need contexts to be implemented cleanly (i.e. with a specific form controller for profile and registration forms). This fits perfectly the scenario proposed above, since we could see it as an exceptional case where the 'add' and the 'edit' forms are (slightly) different.

plach’s picture

Status: Needs work » Needs review

The last submitted patch, entity_forms-1499596-58.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
75.99 KB

Rerolled. Interdiff same as in #58.

fago’s picture

Status: Needs review » Needs work

This is actually splitting the submit-build phase, which may be needed by every action, and the actual submission logic, which is confined in the 'save' submission handler. The base 'submit' handler is not duplicating the submit-build phase, it's just moving it into the controller, which is what you seem to suggest above. This is why I'm a bit confused.

I see - so that moving it into the controller without actually removing it is what actually makes this totally confusing and confused me. So this patch duplicates a lot, without removing existing stuff, while partially using the existing stuff. But then partly, existing stuff is used without incorporating the added stuff, what creates quite a mess and makes the whole already difficult to follow code massively more complex and the whole situation worse.

Instead of that, we need to clean things up and create a central point of control: the controller. Thus, no old submit handler may be called directly any more, best they are incorporated into the controller. Then there should be no back-and-forth between controller and non controller functions, e.g. buildEntiy() vs node_form_submit_build_node() vs node_form_submit() and controller save() and submit(). That flow between those functions is just confusing me. node_form_submit(), node_form_submit_build_node(), node_form_validate() - those all are deprecated by that patch, instead only the node's controller should be invoked.

Thus, I really do think that we need to incorporate moving old handlers to the controller to clean up things here. Sry, I know you've been trying to avoid that, but I don't see that working out otherwise. Also I don't mind reviewing a 200kB patch, in particular if we've Git, revisions and interdiffs. But if you prefer to keep things small, I'd suggest splitting the conversion by entity type instead.

during the build phase the controller retrieves the form array and the actions;
actions are massaged to ensure there is a submit handler for each one rebuilding the entity with the submitted values before any other processing happens;
only the save action gets the validation handler because there is no need to validate the submitted values when deleting the entity (I find very tedious that I'm not allowed to delete a node with an empty title);

If submit-building the entity is activated for a button, validation needs to be activated as well. For deletion, both can/should be passed.

Currently the validation handler is added by the controller in the overridable EntityFormController::actions() method. Are you saying that it should be treated as the submit handler and always added to the 'save' button in the following non-overridable action massaging? If so I'd tend to agree.

I say that all validation/submit handlers clearly need to point to the controller, not in some cases bypass it.

Let me explain my point a little more: after looking better at the current Entity API code, I started thinking that what we are likely to need are not micro-variations of the default entity form, but actually a different form for each operation to be performed on the entity: add/edit (default action), clone, export, delete and so on...

yep, exactly. I'd call the add/edit form the main entity form, others are special operation forms and handled differently.

What I'm proposing is keeping the context system here and renaming it to use a "operation" terminology so that we can have a common logic to register and instantiate a form controller for each needed action. Currently only the user entity type need contexts to be implemented cleanly (i.e. with a specific form controller for profile and registration forms). This fits perfectly the scenario proposed above, since we could see it as an exceptional case where the 'add' and the 'edit' forms are (slightly) different.

export/clone/delete operation form have mostly nothing in common with the main entity form, so I don't think using the same form controller for that makes a lot of sense. Maybe, we could do a small operation-form-controller that in particularly eases customizing text messages? Not sure, but anyway I'd prefer to leave that out of this issue.

Regarding contexts, it might make sense to introduce that as something like "edit_mode" - analogously to view_modes. So "register" and "add" could be different edit-modes, maybe? Anyway, again I'd prefer keeping that out of this issue as well.

plach’s picture

Thus, I really do think that we need to incorporate moving old handlers to the controller to clean up things here. Sry, I know you've been trying to avoid that, but I don't see that working out otherwise. Also I don't mind reviewing a 200kB patch, in particular if we've Git, revisions and interdiffs.

Ok, I'll start moving the various extrernal functions into the controllers. Not sure about factoring code up into the base class: should we postpone that to a follow-up?

If submit-building the entity is activated for a button, validation needs to be activated as well. For deletion, both can/should be passed.

Currently the submit-build phase does not implement any "business" logic, it's just an "extension" of the Form API flow. The actual logic is into the action-specific submit handlers. I'm not sure what to put into a validator that would make sense for all the actions. However I realize that the same validation logic may apply for instance to save and preview actions. Should we have various validate methods, one for each action, like the submit ones?

I say that all validation/submit handlers clearly need to point to the controller, not in some cases bypass it.

Agreed, If there is a validator it totally needs to be on the controller. I was just saying that some actions don't seem to require a validation handler.

export/clone/delete operation form have mostly nothing in common with the main entity form, so I don't think using the same form controller for that makes a lot of sense. Maybe, we could do a small operation-form-controller that in particularly eases customizing text messages? Not sure, but anyway I'd prefer to leave that out of this issue.

I'm not saying that the various operations should have a shared controlloer, I'm just saying that from the DX perspective it would be nice to be able to do something like:

<?php
$export_form = entity_get_form($entity, 'export');
$delete_confirm_form = entity_get_form($entity, 'delete');
?>

Then I might have some operations having a common parent controller, for instance both the AccountFormController (user edit action) and the RegisterFormController (user add action) could inherit from a generic DefaultFormController, but this would not be required at all. There would be only a common "entry point".

Regarding contexts, it might make sense to introduce that as something like "edit_mode" - analogously to view_modes. So "register" and "add" could be different edit-modes, maybe? Anyway, again I'd prefer keeping that out of this issue as well.

Yeah, this somehow reminds view modes, as also @tstoeckler was pointing out. Totally agreed on not addressing it here.

fago’s picture

Ok, I'll start moving the various external functions into the controllers. Not sure about factoring code up into the base class: should we postpone that to a follow-up?

Great! If we convert all entity types at once, I'd do so - maybe we can simplify them a bit as the controller has knowledge about the entity location. But as you prefer - keeping them shouldn't complicate the overall flow much.

Currently the submit-build phase does not implement any "business" logic, it's just an "extension" of the Form API flow. The actual logic is into the action-specific submit handlers. I'm not sure what to put into a validator that would make sense for all the actions.

Submit-building extracts the form values and gives you an updated $entity. If you are using this entity during #submit, you need to add the regular entity #validate as well, or your $entity contains unvalidated values. Only dealing with validated values is the whole idea of the #submit phase :) - if you only partly validate the form you should only access validated form values (that's way non-validated form values are missing from $form_state['values']).

plach’s picture

Status: Needs work » Needs review
FileSize
75.88 KB

Work in progress patch for the bot.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-65.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
131.3 KB
179.33 KB

Here is the big one. Validation handlers fixed as per #62. Contexts/operations still there. Let's see what the bot thinks abut this one.

Status: Needs review » Needs work
Issue tags: -Entity system, -D8MI, -sprint, -language-content

The last submitted patch, entity_forms-1499596-68.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +D8MI, +sprint, +language-content

#67: entity_forms-1499596-68.patch queued for re-testing.

Tests passing locally.

plach’s picture

Assigned: nod_ » plach
Priority: Normal » Major
FileSize
196.53 KB
37.61 KB

[text moved to #75 below to improve readability]

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-70.patch, failed testing.

plach’s picture

Fixed a small issue in the Translation module.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-72.patch, failed testing.

plach’s picture

Assigned: plach » nod_
Priority: Major » Normal
Status: Needs work » Needs review
FileSize
197.03 KB
2.09 KB

The attached patch removes all the occurences of $form['#node'] and friends. Not sure how to proceed with contexts as many things break for users since we cannot have two distinct form ids anymore without them. For now I just changed the terminology from context to operation.

This looks good to go to me. Fago?

plach’s picture

Unintended change.

plach’s picture

FileSize
196.67 KB

Rerolled

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Avoid commit conflicts

The last submitted patch, entity_forms-1499596-77.patch, failed testing.

plach’s picture

FileSize
197.02 KB

#75 was missing.

Please guys let's finalize this and get it in: it's getting awful to reroll :(

Berdir’s picture

Status: Needs review » Needs work

Great stuff, really like this. Looking forward to being able to unify those forms (e.g. save callbacks) to actually save code in the end instead of adding 400 additional lines ;)

Review below, mix of minor things that should be cleaned up (setting to needs work for that) and some notes/questions.

Note that you're missing one entity form in field_test.module, but that has not been converted to an actual classed entity yet, so doesn't make sense yet. Conversion is happening in #1374030: Remove entity_extract_ids() now that we have proper classed entity objects. Note that there's some crazy stuff there like a form with two parallel entities that we probably can't deal with in a generic way.

Did the review of the code in #77, FYI.

+++ b/core/modules/block/block.moduleundefined
@@ -596,7 +598,7 @@ function block_custom_block_save($edit, $delta) {
-  $account = $form['#user'];
+  $account = EntityFormController::getFormInstance($form_state)->getEntity();

What is exactly the reason for using that static method here instead relying on $form_state['controller'] directly?

It's neither a protection against $form_state['controller'] not existing (it would return FALSE in that case and would then explode) nor does it allow to inject/mock anything, it's a hardcoded static function.

$form_state['controller']->getEntity() would IMHO be easier to read.

If there is a reason and I'm not seeing it, care to explain? :)

+++ b/core/modules/comment/comment.admin.incundefined
@@ -278,7 +278,7 @@ function comment_confirm_delete_page($cid) {
  */
 function comment_confirm_delete($form, &$form_state, Comment $comment) {
-  $form['#comment'] = $comment;
+  $form_state['comment'] = $comment;
   // Always provide entity id in the same form key as in the entity edit form.
   $form['cid'] = array('#type' => 'value', '#value' => $comment->cid);
   return confirm_form(
@@ -295,7 +295,7 @@ function comment_confirm_delete($form, &$form_state, Comment $comment) {

@@ -295,7 +295,7 @@ function comment_confirm_delete($form, &$form_state, Comment $comment) {
  * Form submission handler for comment_confirm_delete().
  */
 function comment_confirm_delete_submit($form, &$form_state) {
-  $comment = $form['#comment'];
+  $comment = $form_state['comment'];
   // Delete the comment and its replies.
   comment_delete($comment->cid);

While a valid change, does look a bit unrelated, or are there any side effects of this which aren't visible here?

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.phpundefined
@@ -0,0 +1,373 @@
+  /**
+   * @see Drupal\entity\EntityFormController::form()

I think the standard docblock for this is "Overrides ....".

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.phpundefined
@@ -0,0 +1,336 @@
+      // @todo Consider inlining entity_form_field_validate() here.
+      entity_form_field_validate($entity->entityType(), $form, $form_state);

Note that entity_form_field_validate currently casts $form_state['values'] to an object and uses that as $entity, we need to find a proper fix for that, my entity_extract_ids() issue currently changes that to an entity_create(), which is imho also wrong.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.phpundefined
@@ -0,0 +1,336 @@
+    // @todo Remove this.
+    // Execute legacy global validation handlers.
+    unset($form_state['validate_handlers']);
+    form_execute_handlers('validate', $form, $form_state);

Is this still used in core?

I guess we need a list of follow-up issues to have an overview of the remaining tasks, there are lots of important @todo's in here. Having that list and having a meta-issue or a tag that shows that we have a defined path towards additional improvements could help to get a patch with that many @todo's in. Having a major meta issue with issues for each @todo would show that it's a temporary thing.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.phpundefined
@@ -0,0 +1,336 @@
+    $entity = clone $this->getEntity();
+    // @todo Move entity_form_submit_build_entity() here.
+    entity_form_submit_build_entity($entity->entityType(), $entity, $form, $form_state);

I guess that's what should also do for validate(), maybe we could actually have maintain two entity objects, one with the existing values and one that is updated with the current form values? Something for later, of course.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.phpundefined
@@ -0,0 +1,336 @@
+   * @param $form_state
+   *   The current form state.
+   *
+   * @return Drupal\entity\EntityInterface
+   *   The entity being the current form entity.
+   */
+  public function getEntity() {
...
+   * @param $form_state
+   *   The current form state.
+   * @param Drupal\entity\Entity $entity
+   *   The entity being edited in the current form.
+   */
+  protected function setEntity(EntityInterface $entity) {

@param $form_state is wrong. $entity should be EntityInterface

Powered by Dreditor.

plach’s picture

Status: Needs work » Needs review

FYI, I created a new sandbox to work on the Entity Translation stuff: http://drupal.org/sandbox/plach/1719670.

plach’s picture

@Berdir:

Thanks for the review!

Looking forward to being able to unify those forms (e.g. save callbacks) to actually save code in the end instead of adding 400 additional lines ;)

Yeah, those 400 lines annoy me too, but we should consider that we already have tons of lines that are no longer loaded in the various .pages.inc files :)

What is exactly the reason for using that static method here instead relying on $form_state['controller'] directly?

Well, the main reason is having an API in place: it's true that it does not provide protection/encapsulation or anything, but this way we are exchanging a raw data structure, which becomes an injected dependency (taa-daa), with a true API. People can still use the form state 'controller' key but they do so at their own risk: we might change that key or refactor the getter logic in any moment of the D8 lifecycle and they would not be allowed to complain :)

However if some of the crazy ideas I read in an issue linked somewhere above come true, we might end up with controllers for any form, and we might even get an OOP-fied form state, which would allow us to write something as beautiful as $form_state->getController() :)

While a valid change, does look a bit unrelated, or are there any side effects of this which aren't visible here?

Well, deletion confirmation is an entity form, just a different operation. I thought it might make sense to fix also that one here. Do you think it would be better to leave it out?

Note that entity_form_field_validate currently casts $form_state['values'] to an object and uses that as $entity, we need to find a proper fix for that, my entity_extract_ids() issue currently changes that to an entity_create(), which is imho also wrong. [...]
I guess that's what should also do for validate(), maybe we could actually have maintain two entity objects, one with the existing values and one that is updated with the current form values? Something for later, of course.

Yes, the idea is exactly to use buildEntity() also for validation. However we need to move entity_form_field_validate() in the controller or at least refactor it to get an entity instead of using the form state.

Is this still used in core?

Yes, I don't remember exactly where, but tests fail without it.

I guess we need a list of follow-up issues to have an overview of the remaining tasks, [...]

Totally agreed. As soon as this goes RTBC I'll prepare a list of follow ups.

Berdir’s picture

Changes look good, just one tiny thing...

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -21,7 +21,7 @@ class NodeFormController extends EntityFormController {
    *
-   * @see Drupal\entity\EntityFormController::prepareEntity()
+   * Overrides Drupal\entity\EntityFormController::prepareEntity().

Here was @see actually correct as it's not a hook-like single-line docblock but a reference after that.

plach’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Should still pass with that phpdoc change. Otherwise passed very rigorous reviewers, so let's get this going!

fago’s picture

Status: Reviewed & tested by the community » Needs work

wow, great progress!

Still, as this goes rather deep before going in I think it needs another architectural sign-off from someone involved into the whole entity-form mess, e.g. from sun or effulgentisa!

So let's start with some architectural review first:

First, I must say the code is much clearer and easier to follow now! I like centralizing all the stuff that belongs to a form in a single class, that makes it much easier to see what's there.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+/**
+ * Base for controller for entity edit forms.
+ */
+class EntityFormController {

We need an interface, e.g EntityFormControllerInterface so I can roll my own and people know which methods they can rely on! Then this class should be just a base-form controller I may use.

+++ b/core/modules/block/block.module
@@ -596,7 +598,7 @@ function block_custom_block_save($edit, $delta) {
-  $account = $form['#user'];
+  $account = EntityFormController::getFormInstance($form_state)->getEntity();

I don't see the need for this static-getFormInstance either. The static method cannot know where the controller is either if an implementation customizes its location.

Therefor, I think we should just go with calling $form_state['controller'] as well.

+      // Ensure we always validate and preprocess submitted data before calling
+      // the actual form handlers.
+      array_unshift($element[$action]['#validate'], $validate_callback);
+      array_unshift($element[$action]['#submit'], $submit_callback);

This has confused me a bit when going through the code. When looking at preview I was missing the entity-building and update. Furthermore not every button needs this update, it's actually important that the button decides whether to update data or not *just as* it decides whether to validate data or not. Thus, I think adding the validation and the submit callback should not be automated like that but done manually for each button. That way it's much easier to see what's going to happen when it's pressed + we do not update the entity or validate for an delete button.

ok, then here a regular review of the code:

+++ b/core/includes/form.inc
@@ -266,11 +268,11 @@ function drupal_get_form($form_id) {
+ *     key name or a prefix for the key name. For example, the entity form
+ *     controller classes use $form_state['entity'] in entity forms to store

That's not true, isn't it? I think it's still $form_state[$entity_type]

Then, if I do an entity form with 2 or more entities, it might be desired to work with multiple controller. That wouldn't be doable with this static call.

+++ b/core/modules/book/book.module
@@ -423,7 +425,7 @@ function book_get_books() {
-  $node = $form['#node'];

Yes, this $form['#node'] entity locations are already deprecated for D7. It should be one of the follow-ups to remove all entities sitting in $form.

+++ b/core/modules/entity/entity.module
@@ -470,6 +473,124 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * @param $operation
+ *   (optional) The name of the operation identifying the form controller.

This needs better docs, what are those operations???

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+   * @param $form
+   *   An associative array containing the structure of the form.
+   * @param $form_state
+   *   A reference to a keyed array containing the current state of the form.
+   * @param $entity_type
+   *   The type of the entity being edited.
+   * @param $entity

Docblock misses data types. Others in the controller class too.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+  public final function build(array $form, array &$form_state, EntityInterface $entity) {
Not sure about this: we can still rely on alterations to change anything on the form we wish to. IMO it's important that we somehow codify what is supposed to be changed and what might be dangerous to. A final method is a clear indication, a PHP doc might be ignored or missed.

I don't think we should have final methods? What would happen if I decide to customize that? Why forbid it? Same for other final methods?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+    // Update the form state reference to ensure that any modification to the
+    // entity object is correctly propagated.
+    self::setFormInstance($form_state, $this);
+    return $form;

hm, that looks strange. Why is that needed? Updating the entity object in the controller instance should suffice?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+    $entity = $this->getEntity();

$entity isn't used here?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+   * Processes the submitted form values and updates the entity object.
+   *
+   * This is the default entity object builder function. It is called before any
+   * other submit handler to build the new entity object to be passed to the
+   * following submit handlers.
+   *

Good. Can we incorporate the great docs from http://api.drupal.org/api/drupal/includes!common.inc/function/entity_for... here? Or why is that the default-bulder if buildEntity() does the building? Maybe it should be just clarified what'S the difference to buildEntity(), i.e. it updates the already validated entity in form-state for future rebuilds or saving ?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+    // @todo Move entity_form_submit_build_entity() here.
+    entity_form_submit_build_entity($entity->entityType(), $entity, $form, $form_state);

Let's do so?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -0,0 +1,331 @@
+   * Prepares the entity object.
+   */
+  protected function prepareEntity() {
+    // @todo Perform common prepare operations.
+  }

Does that mean preparing the entity for editing? Then it should say so.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -0,0 +1,436 @@
+    // The entity form controller contains the actual entity being edited, but
+    // we must not update it with form values that have not yet been validated,
+    // so we create an entity clone to use during validation.
+    $node = $this->buildEntity($form, $form_state);

It should be documented at buildEntity that it clones, then it should be natural to go with it here.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -0,0 +1,436 @@
+  /**
+   * Overrides Drupal\entity\EntityFormController::delete().
+   */
+  public function delete(array $form, array &$form_state) {

I'm still not convinced that this belongs into the same controller. In addition as it doesn't seem to be used e.g. for node deletion.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -0,0 +1,133 @@
+    // @todo We should not be calling taxonomy_vocabulary_confirm_delete() from
+    // within the form builder.
+    if ($form_state['triggering_element']['#value'] == t('Delete')) {

Shouldn't that button just have it's own submit handler? But I see that's copied over code, so cleaning it up doesn't belong in here.

plach’s picture

Issue summary: View changes

Added first stab of an issue summary.

plach’s picture

Status: Needs work » Needs review
FileSize
198.08 KB
31.7 KB

Most of the suggestions in #87 are implemented. Some answers:

That's not true, isn't it? I think it's still $form_state[$entity_type]

Nope, see EntityFormController::getEntity().

Then, if I do an entity form with 2 or more entities, it might be desired to work with multiple controller. That wouldn't be doable with this static call.

Would you clarify this? Maybe it was referred to the controller static accessors?

Yes, this $form['#node'] entity locations are already deprecated for D7. It should be one of the follow-ups to remove all entities sitting in $form.

Yes, I left out occurrences not belonging to entity forms to avoid bloating too much the patch.

hm, that looks strange. Why is that needed? Updating the entity object in the controller instance should suffice?

Yes, it's a bit weird but if we don't update also the reference after the form is retrieved from cache, handlers not belonging to the controller will act on a different controller object and thus won't get the updated entity.

$entity isn't used here?

I can't locate that line, but all the occurences of $entity = $this->getEntity() I could spot were using the returned entity.

Let's do so?

Well, entity_form_submit_build_entity() is used also in field tests, it would be easier to address this in a follow up.

I'm still not convinced that this belongs into the same controller.

Almost all converted entity forms have a delete submit handler which redirects to the deletion confirmation form. I think it makes sense to have that one since it is more than likely that we'll end up with a unique shared implementation. Instead I think it would be confusing to have only that one implemented as a regular, procedural submit handler.

In addition as it doesn't seem to be used e.g. for node deletion.

NodeFormController::delete() redirects to the deletion confirmation form and so do the term form controller (also the vocabulary one should). The Profile form controller has a procedural cancel handler that would also fit into this pattern IMHO.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-88.patch, failed testing.

fago’s picture

Thanks, for the update and the answers. Changes look good.

Yes, it's a bit weird but if we don't update also the reference after the form is retrieved from cache, handlers not belonging to the controller will act on a different controller object and thus won't get the updated entity.

Oh, yes. That's problematic, I guess we should make sure there is only one cached controller? E.g. we might want to combine form-state and form in a cache. Maybe let's just an todo there so we don't forget and create a follow-up.

This seems to be only the place left who is hard-coding the location of the controller in $form_state inside the controller. We should avoid that for enabling multiple-entity forms in a single form (what quite some modules do).

That's not true, isn't it? I think it's still $form_state[$entity_type]

Nope, see EntityFormController::getEntity().

Oh, I see. Removing $form_state['node'] is an API change with a rather high impact. As we are moving more and more to entity-generic code I think it makes sense. It should be good visible in the change-notice though.

plach’s picture

Status: Needs work » Needs review
FileSize
459.86 KB
939 bytes

@fago:

This seems to be only the place left who is hard-coding the location of the controller in $form_state inside the controller. We should avoid that for enabling multiple-entity forms in a single form (what quite some modules do).

Do you mean that the 'controller' key should be variable per form/form state?

plach’s picture

FileSize
198.23 KB

Sorry, wrong patch. The interdiff in #91 is still valid.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Maybe let's just an todo there so we don't forget and create a follow-up.

Done

plach’s picture

Just realized that a callback altering entity forms in a generic fashion might need the operation in addition to the controller to know which one it is acting on.

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Assigned: plach » fago

@fago is working on some cleanup

fago’s picture

Assigned: fago » plach
FileSize
200.64 KB
42.45 KB

ok, as said I've worked a bit over this:

  • I noticed that due to $controller->form_state the form state gets serialized *into itself* when it is cached. I've discussed that with plach and we agreed to remove $controller->form_state for now and just pass it on to respective methods always. We may want to simplify the getEntity() calls to do not require the passed on form-state again in the follow-up where we fix the controller being still duplicated to $form and $form_state.
  • I worked over the new interface and default class and tried to improve and clarify docs, as e.g. the relationship between buildEntity() and submit().
  • I incorporated the entity form validation helper and removed some various other glitch where I've thought that it's not necessary anymore, as removing the form-controller-instance re-set on #process. We'll see what the test bot thinks.
  • I also tried to remove customly invoking form-main-level validation handlers, but that seems to be necessary. I guess we should move the a validation-hook here, but let's leave it to a follow-up to introduce all kind of hooks. (altering?, entity-building, validating, submitting)

I've not touched the new entity.module helpers much, in particular the $operation related handling. I'm no fan of the $operation term, while I think introducing the concept makes sense. It's the analogy the view-modes for entity-forms and if field API would support it, it would help a lot to ease customizing registration forms or configuring different form-widgets for searching? I think this needs some more though and probably a rename as it doesn't map to operation (that would be e.g 'delete', 'create'?). Anyway, given that this blocks some further important work I guess we can care about that in a follow-up.

fago’s picture

Assigned: plach » Unassigned
plach’s picture

Changes look good to me, thanks. Obviously I'm not the right person to RTBC this. We may want to wait for @effulgentsia to have a look to the issue.

Berdir’s picture

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.phpundefined
@@ -0,0 +1,257 @@
+      // @todo Use buildEntity() here and make field API leverage it.
+      $pseudo_entity = (object) $form_state['values'];
+      field_attach_form_validate($entity->entityType(), $pseudo_entity, $form, $form_state);

Can we switch to entity_create() here for the time being instead of the object cast so that we have a classed object? I had to do that to get the entity_extract_ids() stuff working in #1374030: Remove entity_extract_ids() now that we have proper classed entity objects.

plach’s picture

The attached patch fixes #99 by using buildEntity() as suggested by the @todo.

fgm’s picture

Might be a good occasion to include "form_modes", the form equivalent of "view modes", as introduced in the entityforms module (not the one in drupal.org/project/entityform, another one I can't get my hands on again, probably a DamZ' sandbox).

plach’s picture

@fgm:

Yes, this has been proposed a couple of times during the discussion. It would certainly be an interesting follow up.

fago’s picture

@#100: Looks good!

@fgm: This patch basically does, see my notes about $operation in the last paragraph of #96.

plach’s picture

FileSize
200.54 KB

Rerolled after entity_extract_ids() has gone.

Status: Needs review » Needs work

The last submitted patch, entity_forms-1499596-104.patch, failed testing.

plach’s picture

Fixed tests. Below a list of proposed follow-ups, if we agree on them, I'll start creating the issues.

  • Build and prepare a basic entity form through the Entity Property API
  • Factor up and consolidate form actions
  • Introduce a common validation workflow for entity forms
    • Move form_state_values_clean() into EntityFormController::validate()
    • Remove global validation handlers
  • Introduce a common submission workflow for entity forms
    • Factor up and generalize action-specific submit handlers
      • Remove form rebuild from vocabulary delete action
      • Make user profile cancellation the regular user delete action
    • Move entity_form_submit_build_entity() into EntityFormController::buildEntity()
  • #1498874: Provide language awareness to entity forms (introduce the form language concept)
  • Add a TestEntityFormController for field test [maybe: use a single test entity for tests so we have to do this only once]
  • Ensure entity forms can be embedded into other forms
  • Introduce form modes as a counterpart of view modes

The nested bullets may or may not deserve a dedicated issue.
 

effulgentsia’s picture

Status: Needs work » Needs review

We may want to wait for @effulgentsia to have a look to the issue.

I haven't reviewed this in detail, but architecturally, I'm very +1 for this. I agree with the form.inc changes and with EntityFormControllerInterface. Even if there's flaws in the patch (and with something this big, there likely are), I'd be ok with letting them get found and dealt with in follow ups. If I manage to get enough time to review this in detail before DrupalCon Munich, I will, but please don't hold this up on me if it manages to get RTBC'd before then, since it would help unblock other D8MI entity issues to be worked on during the conference sprints. Thank you, plach, fago, and everyone else who's helped with this awesome refactor.

plach’s picture

Issue summary: View changes

Improved text

fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks effulgentsia! Let's move on then and complete the list of follow-ups and API changes for the summary:

Here some more points for follow-up tasks:

  • Fix $controllers being cached twice (in $form and $form_state).
  • Move entity_form_submit_build_entity() into the form controller.
  • Introduce hooks for altering, entity-building, validating and submitting an entity form.
  • Ensure multiple entity forms can be embedded into one form.
  • Clarify $operation and the relationship to form modes (as a counterpart of view modes).
  • For validation, I think we want to go for #1696648: [META] Untie content entity validation from form validation and invoke this from the validate handler.
  • Make sure all FAPI callbacks can point to objects (#after_build?).

Then, for the API changes I miss the important point that $form_state['node'] does not longer work.

plach’s picture

@fago:

Thanks! I'll start creating the issues later today, it seems there's some overlap between our lists btw. In a couple of cases I'll just create a stub and I'd ask you if you could complete the OP, because I am not totally clear on how to proceed.

Berdir’s picture

Another follow-up: Move the test_entity form handling in field_test.entity.inc to the form controller. I hope that it should become a nice example on how the form controller works as there should be no/few special cases, unlike our real entities. We could do that in #1616930: Fix the field_test entity type, not sure.

plach’s picture

@Berdir:

I meant exactly this in #106:

Add a TestEntityFormController for field test [maybe: use a single test entity for tests so we have to do this only once]

Sorry, if it was unclear. IMO we should also unify the test entities: now we have one for entity tests and one for field tests. I agree we could use the issue above.

plach’s picture

Issue summary: View changes

Added a list of proposed follow-ups.

plach’s picture

Issue tags: +API change

Updated the issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

This looks awesome!

I have a couple of remarks, but I'd really like to see this patch to land ASAP.

So all of the following can be discussed and handled in one or more follow-ups:

(Also note that I did not read the issue nor previous comments yet, as I wanted to review this from a "fresh" perspective. I'm sorry if I'm duplicating/repeating earlier discussions/comments.)

+++ b/core/modules/block/block.module
+use Drupal\entity\EntityFormController;

+++ b/core/modules/book/book.module
+use Drupal\entity\EntityFormController;

These use statements are added to many files, but the actual/current code calls into an injected $form_state['controller'] now, so they're no longer necessary. I suggest to clean them up after commit.

+++ b/core/modules/comment/comment.admin.inc
 function comment_confirm_delete($form, &$form_state, Comment $comment) {
-  $form['#comment'] = $comment;
+  $form_state['comment'] = $comment;

 function comment_confirm_delete_submit($form, &$form_state) {
-  $comment = $form['#comment'];
+  $comment = $form_state['comment'];

Delete confirmation forms are not converted in this patch yet? In any case, let's make sure that we're consistently using $form_state['entity'] everywhere. That can be happily cleaned up in a follow-up, too, though.

+++ b/core/modules/comment/comment.module
+function comment_add($node, $pid = NULL) {
+  $values = array('nid' => $node->nid, 'pid' => $pid, 'node_type' => 'comment_node_' . $node->type);
+  $comment = entity_create('comment', $values);
+  return entity_get_form($comment);
+}

Shouldn't this pass the full $node object sorta as an additional "context" into entity_get_form()? That's a more complex case, which we can happily discuss in a follow-up.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -0,0 +1,378 @@
+  public function form(array $form, array &$form_state, EntityInterface $comment) {
...
+    return parent::form($form, $form_state, $comment);

It looks a bit odd that we're calling into the base implementation at the end instead of at the beginning. However, we can discuss that in a follow-up.

+++ b/core/modules/entity/entity.module
+function entity_form_id(EntityInterface $entity, $operation = 'default') {
...
+function entity_form_state_defaults(EntityInterface $entity, $operation = 'default') {
...
+function entity_form_submit(EntityInterface $entity, $operation = 'default', &$form_state = array()) {

It's not quite clear to me why these functions do not live on the base EntityFormController, but I'm happy to discuss that in a follow-up.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
+  protected function init(array &$form_state, EntityInterface $entity) {
...
+  public function form(array $form, array &$form_state, EntityInterface $entity) {
...
+  protected function actionsElement(array $form, array &$form_state) {
...
+  protected function actions(array $form, array &$form_state) {

Some of the EntityFormController's method names are continuing the bad & ugly method naming practice that we began in the Entity class. I'm really not happy about some of these method names, but I'm fine with fixing their names in a follow-up.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
+    $this->setEntity($entity, $form_state);

I think it would make sense to store a reference to $form_state in a protected $this->formState property within the EntityFormController to avoid having to pass the $form_state argument all over the place. However, we can happily do that in a follow-up.

plach’s picture

@sun:

These use statements are added to many files, but the actual/current code calls into an injected $form_state['controller'] now, so they're no longer necessary. I suggest to clean them up after commit.

Yeah, those are leftovers of when we had static methods to access the controller in the form state. Agreed on rolling a quick follow up patch after commt.

Delete confirmation forms are not converted in this patch yet?

Not yet, I felt that 200K was a sensible limit for the patch size :)

In any case, let's make sure that we're consistently using $form_state['entity'] everywhere. That can be happily cleaned up in a follow-up, too, though.

Sure, I guess every entity form should rely on the same internal key in the form state, unless there's a good reason to make an exception. I'll add a follow up to the list for this.

Shouldn't this pass the full $node object sorta as an additional "context" into entity_get_form()? That's a more complex case, which we can happily discuss in a follow-up.

Do you mean using a dynamic argument list in entity_get_form() to pass further parameters to the controller? That might make sense.

It looks a bit odd that we're calling into the base implementation at the end instead of at the beginning.

Not sure why, I've always seen such things in OOP code. However we can definitely address this in the issue merging/factoring-up the various form() methods.

It's not quite clear to me why these functions do not live on the base EntityFormController, but I'm happy to discuss that in a follow-up.

Those are at the same level of entity_get_form() (actually it depends on most of them), hence we cannot move them into the controller without introducing some weird intantiation pattern or making them static methods, which have been banned from the base controller implementation.

Some of the EntityFormController's method names are continuing the bad & ugly method naming practice that we began in the Entity class. I'm really not happy about some of these method names, but I'm fine with fixing their names in a follow-up.

We totally need to address this (somewhere else): I saw @Dries complaining about entity method names in two or three different issues already :)
For now I just tried to stay consistent.

I think it would make sense to store a reference to $form_state in a protected $this->formState property within the EntityFormController to avoid having to pass the $form_state argument all over the place. However, we can happily do that in a follow-up.

This was the way we did it until #96, where we discovered this approach has problems. There are a couple of related follow ups planned.

Crell’s picture

It looks a bit odd that we're calling into the base implementation at the end instead of at the beginning.

Not sure why, I've always seen such things in OOP code. However we can definitely address this in the issue merging/factoring-up the various form() methods.

That's completely legit. You can call parent::whatever() at the start, end, or even middle of an overriding method depending on what's appropriate for your use case. Beginning is just the most common. (I haven't followed the code here enough to have any sense of whether that's appropriate here or not, but there's nothing inherently wrong with it.)

Crell’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Added follow up issues.

plach’s picture

Assigned: Unassigned » plach

Merged the suggested follow ups from #106, #108, #110 and #113 in the issue summary.

Creating the issues.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue tags: +Entity forms

tagging

plach’s picture

Issue summary: View changes

Updated follow-up list.

Dries’s picture

I looked at this and it looks promising. I do want to spend a bit more time on this though, before committing it.

catch’s picture

I started reviewing this but lost my dreditor session. Only had minor complaints so if Dries beats me to finishing his review I'd not want to hold it up.

sun’s picture

Would be lovely to have this in core before the weekend, since we're having an Entity API + Field API + Config system + D8MI sprint.

catch’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -788,6 +793,15 @@ function comment_node_page_additions(Node $node) {
 /**
+ * Returns a rendered form to comment the given node.
+ */
+function comment_add($node, $pid = NULL) {
+  $values = array('nid' => $node->nid, 'pid' => $pid, 'node_type' => 'comment_node_' . $node->type);
+  $comment = entity_create('comment', $values);
+  return entity_get_form($comment);

We could type hint $node, and there's no docs for the function parameters.

+++ b/core/modules/entity/entity.moduleundefined
@@ -418,16 +421,123 @@ function entity_page_label(EntityInterface $entity, $langcode = NULL) {
+  else {
+    throw new EntityMalformedException("Missing form controller for '$entity_type', operation '$operation'");

I think this exception was added for entities with no bundles etc., not for entity types themselves. We should probably have two different exception classes for those?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.phpundefined
@@ -0,0 +1,256 @@
+  /**
+   * Initialise the form state and the entity before the first form build.

British spelling mwahahaha.

The only thing that really sticks out to me is the submit() vs. save() methods. We usually call $entity->save() in a form submit handler, but now we do that in the save() method (assuming it's overriding the default class since it's not in the interface at all). I think the default class could use more docs on the save() method to indicate what it's for.

None of this is serious to stop me committing it so I'll do that later tonight or some time early tomorrow.

plach’s picture

Rerolled the patch after the latest commits and took care of the clean ups mentioned in #113 and #122, plus a couple I found myself meanwhile. The actual code is untouched so keeping the RTBC status.

I think this exception was added for entities with no bundles etc., not for entity types themselves. We should probably have two different exception classes for those?

I switched to InvalidArgumentException, which seems appropriate to me. We may want to introduce more meaningful exceptions in another issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, entity_forms-1499596-123.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
198.44 KB
421 bytes

Forgot a backslash :(

aspilicious’s picture

backslash isn't needed, I think...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Should be back to RTBC. We can remove that backslash later if that is better.

catch’s picture

Title: Introduce a basic entity form controller » Change notification for: Introduce a basic entity form controller
Category: feature » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Alright. This is blocking a fair bit of work, and looks in very good shape. Thanks for fixing some of the follow-ups prior to commit.

I've committed/pushed to 8.x now, leaving this open for the change notification etc.

If there's follow-up issues found for bugs or clean-up for this, it'd be good to open new issues for those but link to them from this one.

plach’s picture

Status: Active » Needs review

Cool, thanks!

Here are a couple of change records:

http://drupal.org/node/1734540
http://drupal.org/node/1734556

plach’s picture

@catch:

If there's follow-up issues found for bugs or clean-up for this, it'd be good to open new issues for those but link to them from this one.

Agreed. Maybe we can roll a quick follow-up if @Dries has any pending concern that does not strictly require a dedicated issue.

vasi1186’s picture

Title: Change notification for: Introduce a basic entity form controller » Introduce a basic entity form controller
Category: task » feature
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -sprint

I've read the change notices, they look fine for me, I easily understood them.

tim.plunkett’s picture

Title: Introduce a basic entity form controller » Change notification for: Introduce a basic entity form controller
Category: feature » task
Priority: Major » Critical
Status: Fixed » Active

I think the change notices are still incomplete, it took me more than a couple minutes to figure out where user_profile_form() went.

Maybe a list of the entity form functions that were removed? So someone googling "where did user_profile_form go in D8" can find the info?

It should also have an example of using this in a hook_menu, I think.

fago’s picture

Issue tags: +sprint
jvns’s picture

I've updated the change notices to be more clear, including adding a list of entity form functions that were removed. Still no hook_menu example, though.

plach’s picture

Status: Active » Reviewed & tested by the community

Looks good to me.

Tim?

jvns’s picture

Status: Reviewed & tested by the community » Needs review

Added a hook_menu() example

jvns’s picture

Status: Needs review » Reviewed & tested by the community

oops, fixing issue status

tim.plunkett’s picture

Title: Change notification for: Introduce a basic entity form controller » Introduce a basic entity form controller
Category: task » feature
Priority: Critical » Major
Status: Reviewed & tested by the community » Fixed

Awesome! Thanks.

Gábor Hojtsy’s picture

Thanks everyone! Amazing!

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue tags: -sprint

Should be off the sprint, thanks all!

Gábor Hojtsy’s picture

Bonus: this is getting a lot more test coverage in #1757232: Improve test coverage for the entity form controller once committed :)

Status: Fixed » Closed (fixed)

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

chx’s picture

Issue tags: -Avoid commit conflicts
chx’s picture

try #2

chx’s picture

Issue summary: View changes

Updated issue summary.

donquixote’s picture

Version: 8.0.x-dev » 9.x-dev

I found this issue after some git blame.

In form_builder() at the time of the patch, but nowadays in FormBuilder::doBuildForm():

-      $element = $process($element, $form_state, $form_state['complete_form']);
+      $element = call_user_func_array($process, array(&$element, &$form_state, &$form_state['complete_form']));

Is there a reason why this uses call_user_func_array() instead of call_user_func()?
I think the latter would make the by-reference stuff easier, right?

In general, call_user_func() is good for a known number of args, whereas call_user_func_array() is good for an unknown number of args. I would expect call_user_func() to be faster because it is less dynamic and does not make you deal with arrays. But I have no evidence for this atm.

Why am I digging this up?
- In #2096731: form_builder() pointlessly passes $element by reference to #process callbacks it was questioned why we pass the $element by reference, when we are really interested in the return value.
- In #784874-13: Add ability to insert FAPI wrapper element without modifying #parents of children I identified a misbehaving #process callback, FormTestFormStateValuesCleanForm::cleanValue().

These obversations led me to a closer look at this mechanic.
Maybe participants of this issue still remember why call_user_func_array() was used.

donquixote’s picture

Version: 9.x-dev » 8.1.x-dev

Sorry for changing the version. I think this happened automatically because 8.0.x was no longer available. Maybe a release tag should be chosen instead?

donquixote’s picture

Version: 8.1.x-dev » 8.0-alpha2

8.0-alpha2 is the first/oldest available tag in the select list for the 8.0.x branch. The patch (commit ea25df8) was applied before 8.0-alpha1, but this is not in the list.

donquixote’s picture

Sorry for the noise.

The call_user_func_array() is required because call_user_func() does not pass parameters by reference.
http://stackoverflow.com/questions/13798468/how-can-i-pass-reference-to-...

If a function expects by-reference parameters, then call_user_func() will result in
"Warning: Parameter 1 to foo() expected to be a reference, value given in .. on line .."
https://3v4l.org/5ojUk

I could have figured this out by myself before posting.

donquixote’s picture

But we could have stayed with the $process($element, $form_state, $form_state['complete form']) syntax from D7.

This works for all callback types except 'C::foo'.
E.g. it works for ['C', 'foo'], [new C, 'foo'], and lambdas.
And it handles references just fine.
https://3v4l.org/NiBos

In PHP 7 it also handles 'C::foo', but we are not there yet.
https://3v4l.org/vbJRt

So, why was this changed to call_user_func_array()? This thread is very long, but I do not find any single mention of this function with find-in-page.

Nowadays, if we are already calling $form_state->prepareCallback($callback), we could make sure that 'S::foo' is translated into ['S', 'foo'].

------------

Note: I only want to know about the reasons for the commit from this issue.
If we actually want to do something about it, a new issue shall be opened.