Problem/Motivation

Entity form handlers have no way to decide if the $entity object set by \Drupal\Core\Entity\HtmlEntityFormController::getFormObject() (and derived from a Request object) is the one that they actually need to work with.

This is a problem in cases like the field_storage_config form whose URL (e.g. admin/structure/types/manage/article/fields/node.article.body/storage) does not contain the id of the entity type they need to work with. In this particular case, we're getting around this limitation because \Drupal\field_ui\Form\FieldStorageEditForm is not the "official edit form handler" for the field_storage_config entity type, but we want change this fact in #552604: Adding new fields leads to a confusing "Field settings" form (a long-standing usability issue in Field UI).

Proposed resolution

Move the responsability of determining the $entity from the request from \Drupal\Core\Entity\HtmlEntityFormController to the EntityForm object itself.

Further to that, the patch also moves away from using request attributes and switches to a route match object instead, making it a sub-issue of a critical task: #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead

User interface changes

Nope.

API changes

Two new methods, getEntityFromRouteMatch() and setEntityManager(), are added to \Drupal\Core\Entity\EntityFormInterface.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it prevents entity form handlers from working properly if the URL from which they are accessed doesn't contain the entity ID
Issue priority Major because it blocks a long-standing usability issue: #552604: Adding new fields leads to a confusing "Field settings" form
Prioritized changes The main goal of this issue is to unblock a usability issue. (UI/UX changes are not frozen)
Disruption None.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
5.99 KB

Something like this is what I have in mind.

dawehner’s picture

Drupal is a complex beast, so we need that flexibility

+1, even its a little bit odd that HtmlEntityFormController really does not do a lot anymore.

larowlan’s picture

Is this an opportunity to update to use route match instead, while we're touching those bits?

dawehner’s picture

Sounds like a good idea, but therefore we really bettter change the API to pass in a route match object.

Note: This would make this issue a subissue from #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead which needs a review :P </ads>

amateescu’s picture

FileSize
13.95 KB
11.46 KB

Something like this?

Status: Needs review » Needs work

The last submitted patch, 5: 2399219-5.patch, failed testing.

dawehner’s picture

Note: you don't need to inject it .. the route match can be accessed from the controller itself, much like the request object.

amateescu’s picture

Status: Needs work » Needs review
FileSize
11.51 KB
8.41 KB

Nice! Thanks for the tip :)

amateescu’s picture

FileSize
11.52 KB
635 bytes

Missed a spot.

The last submitted patch, 8: 2399219-8.patch, failed testing.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -1513,6 +1513,13 @@ public function setOperation($operation) {
+  /**
+   * {@inheritdoc}
+   */
+  public function setEntityManager($entity_manager) {
+    return $this;
+  }
+

Just because the other implementations are horrible, let's don't add bad code here

amateescu’s picture

FileSize
12.67 KB
1.66 KB

Sure thing :)

amateescu’s picture

Is there anything else I can do to get this RTBC? I'd love to move forward with the Field UI stuff :)

Berdir’s picture

Status: Needs review » Needs work

Looks good to me, found just one small nitpick.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityFormInterface.php
    @@ -65,6 +63,19 @@ public function getEntity();
    +   * @return \Drupal\Core\Entity\EntityInterface
    +   *   The entity object as determined from the passed-in Request.
    

    ... route match. :)

  2. +++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.php
    @@ -66,19 +66,19 @@ protected function getFormArgument(Request $request) {
    +    // Allow the entity form handler to determine the entity object from a given
    +    // request.
    +    $entity = $form_object->getEntityFromRouteMatch($route_match, $entity_type_id);
    

    just "entity form" would be enough I think, but not feeling strong about it.

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.67 KB
1.43 KB

Ugh, I thought I got`em all. Thanks for reviewing :)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks like @dawehner is OK with the patch, looks fine to me as well.

There are some small API changes here, but I doubt they are going to break anything as they are pretty internal and they are a part of the critical issue #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead, so that should be OK.

The issue summary could use a quick update for request => route match and explain why the API changes in existing code are related and OK ?

amateescu’s picture

Issue summary: View changes

Updated the issue summary per #16, without mentioning FormController::getFormArgument() and FormController::getFormObject() since they are not public methods.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Less usage of the request object - nice one. Less usage of the request object - nice one. Committed eb25fab and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed eb25fab on 8.0.x
    Issue #2399219 by amateescu: Allow entity form handlers to determine the...

Status: Fixed » Closed (fixed)

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