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
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. |
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff.txt | 1.43 KB | amateescu |
#15 | 2399219-15.patch | 12.67 KB | amateescu |
#12 | interdiff.txt | 1.66 KB | amateescu |
#12 | 2399219-12.patch | 12.67 KB | amateescu |
#9 | interdiff.txt | 635 bytes | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedSomething like this is what I have in mind.
Comment #2
dawehnerDrupal 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.
Comment #3
larowlanIs this an opportunity to update to use route match instead, while we're touching those bits?
Comment #4
dawehnerSounds 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>
Comment #5
amateescu CreditAttribution: amateescu commentedSomething like this?
Comment #7
dawehnerNote: you don't need to inject it .. the route match can be accessed from the controller itself, much like the request object.
Comment #8
amateescu CreditAttribution: amateescu commentedNice! Thanks for the tip :)
Comment #9
amateescu CreditAttribution: amateescu commentedMissed a spot.
Comment #11
dawehnerJust because the other implementations are horrible, let's don't add bad code here
Comment #12
amateescu CreditAttribution: amateescu commentedSure thing :)
Comment #13
amateescu CreditAttribution: amateescu commentedIs there anything else I can do to get this RTBC? I'd love to move forward with the Field UI stuff :)
Comment #14
BerdirLooks good to me, found just one small nitpick.
... route match. :)
just "entity form" would be enough I think, but not feeling strong about it.
Comment #15
amateescu CreditAttribution: amateescu commentedUgh, I thought I got`em all. Thanks for reviewing :)
Comment #16
BerdirThanks, 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 ?
Comment #17
amateescu CreditAttribution: amateescu commentedUpdated the issue summary per #16, without mentioning
FormController::getFormArgument()
andFormController::getFormObject()
since they are not public methods.Comment #18
alexpottLess 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.