Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
hook_entity_extra_field_info()
allows to declare extra fields for forms as well as displays. Currently, the Extra Field module hardcodes implementations to display:
ExtraFieldFormatterManager::fieldInfo()
$info[$entityType][$bundle]['display'][$fieldName] = [
'label' => $definition['label'],
'weight' => $definition['weight'],
'visible' => $definition['visible'],
];
Proposed resolution
Allow ExtraField plugins to be a 'display' or 'form' implementation.
Remaining tasks
Write a patchCommunity review and testingUpdate documentation- Include a few plugin examples
- Commit
User interface changes
Extra Field Formatters will be available in the Form UI admin interface.
API changes
Only additions to the API:
- A new annotation,
@ExtraFieldForm()
, with the same fields as@ExtraFieldDisplay
, - A new hook,
hook_extra_field_form_info_alter()
to alter@ExtraFieldForm()
annotation data, - A new plugin manager service,
plugin.manager.extra_field_form_display
which maps to\Drupal\extra_field\Plugin\ExtraFieldFormManager
, which conforms to a new\Drupal\extra_field\Plugin\ExtraFieldFormManagerInterface
, - A
\Drupal\extra_field\Plugin\ExtraFieldFormBase
abstract class to provide a default implementation for extra form field plugins, and, - A
\Drupal\extra_field\Plugin\ExtraFieldFormInterface
interface to provide a specification for extra form field plugins.
Data model changes
No changes.
Comment | File | Size | Author |
---|---|---|---|
#39 | 2882061-39.patch | 30.1 KB | Sutharsan |
| |||
#39 | interdiff-2882061-38-39.txt | 9.8 KB | Sutharsan |
#37 | 2882061-37.patch | 23.84 KB | Sutharsan |
| |||
#37 | interdiff-2882061-36-37.txt | 5.62 KB | Sutharsan |
#36 | 2882061-36.patch | 18.6 KB | Sutharsan |
|
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedWith class naming I anticipated on having separate plugins and plugin managers for EF Formatters and EF Widgets. That is the reason behind a hard coded 'display' key in ExtraFieldFormatterManager::fieldInfo(). This does not mean that we can not integrate formatters and widgets in one plugin (manager).
I'm open to suggestions. But once this module reaches beta, I will not allow disruptive changes anymore. @idebr If you plan to work on this issue (you have not assigned it to yourself) let me know. Beta is not too far away anymore.
Comment #3
idebr CreditAttribution: idebr at ezCompany commentedAh, having separate managers would make sense since a vanilla implementation (without Extra Field) would also require implementing different hooks (hook_entity_view vs. hook_form_alter). However the current naming scheme does not indicate if the plugin implements a 'display' or 'form' extra_field. Perhaps these naming conventions could be updated to more accurately reflect their implementation?
Current implementation:
Suggested implementation:
and possible (future) implementation:
Form extra fields are currently not in scope for the project I am working on, so I will probably not be working on this feature any time shortly.
Comment #4
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedI did choose 'Formatter' and 'Widget' as part of the ExtraField formatters to be in line with core's field formatter and field widget plugins.
Comment #5
idebr CreditAttribution: idebr at ezCompany commentedFor what it's worth: a (senior) colleague today expected ExtraFieldFormatterBase to implement Drupal\Core\Field\FormatterInterface and was thoroughly confused about the class naming, so I'm still proposed to renaming these classes to prevent confusion with FieldWidget / FieldFormatter plugins.
Comment #6
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #7
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThis issue got derailed with the naming discussion. To rename the plugin I've created (and fixed) a new issue: #2894463: Rename plugins to be more developer friendly.
Comment #8
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #9
idebr CreditAttribution: idebr at ezCompany commentedSorry for the confusion, this issue not about configurable extra fields. Its purpose is to support extra fields in forms.
Comment #10
andrew_tspkhHi there
I've added new functionality to provide the new extra field plugins for using in the entity form displays.
The example plugin you can find here:
extra_field/modules/extra_field_example/src/Plugin/ExtraField/Form/ExampleFormAllNodes.php
Please, review.
Thanks!
Comment #12
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThanks for contributing. Extra Field was made to allow this addition, to here we go ;)
Can array_merge_recursive() be used here?
We should be carefull on naming. Not to confuse the two plugin types.
This is duplicate code. Add a todo for later refactoring.
Also supportedEntityBundles, allEntityBundles, fieldName, entityBundleKey
Comment #13
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedSorry, no time for more review now.
Comment #14
andrew_tspkhhi, thank you for reviewing!
Here is new patch with improvements :-)
Comment #16
idebr CreditAttribution: idebr at ezCompany commentedI created a new issue to fix the failure on the test runner on the Drupal infrastructure: #2986211: Missing @group annotation in Drupal\Tests\extra_field\Functional\ExtraFieldBrowserTestBase
Comment #17
andrew_tspkhComment #18
mparker17Since #2986211: Missing @group annotation in Drupal\Tests\extra_field\Functional\ExtraFieldBrowserTestBase was committed earlier today, I re-ran the test, which shows up green.
Furthermore, I was able to successfully create an ExtraFieldForm plugin which extends ExtraFieldFormBase and have it show up on the form for the entity type and bundle I specified in the description.
Overall the code looks clear and conforms to coding standards and best practices. That being said...
1. I did notice the patch adds a bunch of TODO comments: were we planning to solve any/some/all of those in this patch, or fix them in future patch(es)?
2. In
modules/extra_field_example/src/Plugin/ExtraField/Form/ExampleFormAllNodes.php
, there's a call on line 49 todrupal_set_message('Custom submit was triggered.')
. The Coder module's DrupalPractice linter says: "Messages are user facing text and must run through t() for translation". Since this code is in an example file, I don't know if it's worth fixing this issue.3. On that note, drupal_set_message() was deprecated in favor of MessengerTrait in 8.5. Since this code is in an example file, and
drupal_set_message()
will stick around until D9 (since removing it would break backwards compatibility), I don't know if it's worth fixing this.Comment #19
mparker17Boldly marking this as RTBC; feel free to bump it back if we want to fix the issues in #18.
Comment #21
mparker17Could I request an addition? Could we pass the
$form_state
fromextra_field_form_alter()
through\Drupal\extra_field\Plugin\ExtraFieldFormManager::entityView()
to the plugin'sformView()
?When I am displaying a form extra field on an \Drupal\Core\Entity\EntityForm, I'd like to be able to access the entity (which is usually at
$form_state->getBuildInfo()['callback_object']->getEntity()
on EntityForms).As a work-around in my specific case, I can look for parameters in the route, but that isn't ideal, because it wouldn't work for, say, inline entity forms, or forms in blocks.
(note that I'm not asking for $form to be passed through - it would also work for my use case, but I suspect someone might be tempted to modify the form directly in formView(), rather than returning what they wanted added)
Is anyone willing to update the patch in #14? If so, I can review and RTBC! If not, I'm happy to write the patch, but someone else will have to review and RTBC for me!
Comment #22
andrew_tspkhHi there
Yes @mparker17 it is good idea.
I've added $form too, but not through the link - so user cannot change the form :-)
Patch updated.
Comment #24
andrew_tspkhComment #25
andrew_tspkhComment #26
mparker17Woot, I'll review it shortly, thanks for the prompt response!
Comment #27
mparker17Adding an interdiff between the patch in #14 and #24 for review (note this skips the patch in #22).
Comment #28
mparker17Code review of the patch in #24: Looks great! The patch is clear, it conforms to coding standards, and it makes good use of the Drupal API.
Manual testing on a fresh install of Drupal 8.6.x core: Works as expected!
Manual testing on my project: Works as expected!
Excellent work! Back to RTBC! Thank you very much @andrew_tspkh!
(I have updated the issue summary with some additional information now that the patch is built)
Comment #29
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedVarious renaming while getting acquainted with the code. See patch.
Interface changes:
- ExtraFieldFormManagerInterface::entityView to ExtraFieldFormManagerInterface::entityFormAlter (because it is meant to be used in hook_form_alter())
- ExtraFieldFormInterface::formView to ExtraFieldFormInterface::view (to be consistent with \Drupal\Core\Field\FormatterInterface::view)
I agree with using a trait for the common code. I will add the trait to the existing code before this patch lands.
Needs review for more in-depth check.
Comment #30
idebr CreditAttribution: idebr at ezCompany commentedI checked some real world implementations of form extra fields. This implementation would not be able to achieve a similar form structure:
Redirect module:
Scheduler module:
From a developer's perspective, it would be nice to:
$form[$fieldName]
array structure. This is currently hardcoded to a'#type' => 'container',
, but real world implementations suggest this is not always the case.The method name
ExtraFieldFormInterface::view()
is consistent withExtraFieldDisplayInterface::view()
, but since it uses different arguments perhaps a different method name would make it easier to distinguish the two. MaybeformElement()
to match\Drupal\Core\Field\WidgetInterface::formElement
?Comment #31
idebr CreditAttribution: idebr at iO commentedAttached patch implements the following changes:
view()
return value is now assigned to the root of the$form
elementsetEntity()
andgetEntity()
to the ExtraFieldFormInterface so you have access to the current entity.$form
is now passed to the plugin by reference, since this variable is available from hook_form_alter() by reference.Comment #32
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedMoving to to a new feature branch to allow new functionality.
Comment #33
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedRe-rolled the patch after #3071507: Prevent duplicate code when adding an EF form manager got committed.
Comment #34
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedGood suggestion.
In the patch some personal code style changes: Variables in camel case, early returns and added empty lines.
Comment #35
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedReplaced
ExtraFieldFormInterface::view
byExtraFieldFormInterface::formElement
.I played around with some plugins and realized that ExtraFieldForm plugins can be used form alter too. Not sure if I like it, but in the same manner ExtraFieldDisplay plugins can modify the content of the host entity object. It just comes with passing data around by reference.
The code is getting mature, but needs more tests before I want to commit it.
Comment #36
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #37
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedAdding one kernel and one functional test.
Comment #38
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedCan someone provide me with some practical examples for using EF form plugins? Perhaps one or two can be turned into an example plugin.
Further, when would you use an extra field form plugin and when use an hook_form_alter implementation?
Comment #39
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedRenamed
$formState
to$form_state
in ExtraFieldFormInterface for consistent developer experience.Updated the readme with documentation for form type extra fields.
Comment #40
idebr CreditAttribution: idebr at iO commented#38 If I have an element to add to the form, I would use an extra field form element. This API is not new; Drupal Core already enables it. This module only provides a Plugin wrapper.
Below is a form element that integrates the Group module with the node form. It uses both a form submit handler as as well an entity builder. Either could be used for an example plugin.
Let's not use calls to the \Drupal:: namespace, but use dependency injection instead. Perhaps use messengerTrait here?
Comment #42
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedLet's get this one in and extend with examples and more tests is follow-up issues.