Problem/Motivation
Sometimes, we need a way to override a form_mode based on some conditions. The common use case is to apply different form modes for different user roles.
The workaround was possible via hook_entity_form_display_alter hook with passing along $entity. That was the original idea of this issue.
Proposed resolution
The initial resolution was to add entity object to the $context array.
However, in #14 @ amateescu came up with a better idea:
It would be better to introduce a new hook: hook_entity_form_mode_alter(), to mirror the one we have for altering view modes: hook_entity_view_mode_alter().
So let's do this!
Remaining tasks
Review MR - https://git.drupalcode.org/project/drupal/-/merge_requests/188
Review CR - https://www.drupal.org/node/3189884
Commit :)
User interface changes
-
API changes
As mentioned in the CR, there is a new hook hook_entity_form_mode_alter, that allows to alter form_mode for given entity.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 2899847-14.patch | 750 bytes | amateescu |
Issue fork drupal-2899847
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2899847-entity-form-display-context
changes, plain diff MR !188
Comments
Comment #2
Johnny vd Laar commentedComment #5
cameron prince commented+1 for this... I've tested the patch and it works as described.
This is a critical need for fully exploiting form modes.
As non-intrusive as this change is, would tests be required?
Comment #6
alexpottThanks for working on this.
In order to commit a new feature we need an automated to test to prove that we've added what we think we have and ensure that we don't break it in the future. For more information about writing tests in Drupal 8 see the following links:
Also this is a change so developers need to be informed about the change so a change record is required. Plus we need to update the documentation of hook_entity_form_display_alter() in core/lib/Drupal/Core/Entity/entity.api.php
Comment #13
matroskeenHere is a list of applied actions:
1) The merge request was opened with changes from #2, updated documentation of
hook_entity_form_display_alter()and adding a reference to$context['entity']in test. I was wondering if we need some extra test coverage if we already have testEntityFormDisplayAlter().2) IS was updated according to the template;
3) Draft change record was created: https://www.drupal.org/node/3189884 (this is my first change record, so I'd like someone to review)
Comment #14
amateescu commentedI don't think
hook_entity_form_display_alter()is the right place to change the form display, it happens a bit too late in the "render display collection process", most importantly, after we possibly create a fresh runtime object if a form display for the given form mode doesn't exist.It would be better to introduce a new hook:
hook_entity_form_mode_alter(), to mirror the one we have for altering view modes:hook_entity_view_mode_alter().Attaching a patch with the place where I think it should be invoked.
Comment #15
amateescu commentedComment #16
matroskeen@amateescu, thank you for your input, it makes perfect sense. I've updated the MR, you're welcome to review it again.
I removed the
$contextbecause it seems redundant. I also noticed that $context is always empty inhook_entity_view_mode_alterand created a separate issue - #3193131: $context in hook_entity_view_mode_alter is always empty.Also, changed the issue title and description to align it with the proposed resolution and merge request.
Comment #17
amateescu commentedLooks great to me! Thanks for following up with it, @Matroskeen :)
Comment #19
catchLooks great! Committed f049151 and pushed to 9.2.x. Thanks!
Comment #22
donquixote commentedI like the "better idea".
However, the original idea was also good, and I don't see them as mutually exclusive.
Perhaps we can open a follow-up issue?
In my own use case, I want to change the widget type for a base field depending on the current value stored in the entity.
I think this can be done in other ways (creating a custom widget type), but it still makes sense to me to expose this in the hook.