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.
Proposed resolution
The widget form element only contains the field name which is not enough to retrieve the field configuration at later hooks. For example in the chosen module we would need the field configuration to be available in the pre render function. To be able to call the \Drupal\field\Entity\FieldConfig::loadByName() function we need the above data.
Proposed resolution
The entity type and bundle values can be added in the form function of the WidgetBase Class.
Comment | File | Size | Author |
---|---|---|---|
#15 | the_widget_form_element-2678824-14.patch | 3.15 KB | mr.york |
#13 | the_widget_form_element-2678824-13-only-test.patch | 2.37 KB | mr.york |
#2 | the_widget_form_element-2678824-2.patch | 802 bytes | mr.york |
Comments
Comment #2
mr.york CreditAttribution: mr.york at Agence Inovae commentedAttache patch.
Comment #3
mr.york CreditAttribution: mr.york at Agence Inovae commentedComment #4
Aron NovakI did a quick test, also reviewed the code. It's a trivial addition to expose more information for later, the only potential question is if there's already a way to grab this info, if not, it should be fine.
Comment #5
catchhook_field_widget_form_alter() has access to the widget plugin instance, which in turn would let you get the field definition via $context['widget']->fieldDefinition()
Can you explain why that isn't enough?
If it isn't, could use test coverage I think.
Comment #6
jhedstromI may be missing something, but when I started looking into #5 as an alternative approach, I don't think the protected
WidgetBase::$fieldDefinition
property is available (no getter method). There is a protected method to access to the field settings viaWidgetBase::getFieldSettings()
, but even if that were a public method it wouldn't be usable since the method isn't part of any interface.Comment #7
mr.york CreditAttribution: mr.york at Agence Inovae commentedThe approach suggested in #5 would only be a workaround for the problem.
In Drupal 8 its not enough that we know the name of the field, to load the informations necessary in a hook.
(https://api.drupal.org/api/drupal/core!modules!field!src!Entity!FieldCon...)
To get the field configuration in a hook, we need to know the entity machine name and the bundle of the field.
Is there a good reason why these informations are not getting into the elements variable?
In the hook suggested by #5 the $context['widget']->fieldDefinition is protected, and so its methods cannot be called.
And I could not find any method in the WidgetBase class that would help in this situation either.
Comment #8
catchOK that's a good point that $this->fieldDefinition isn't available - but why not add public function getFieldDefinition() to WidgetFormBase() then?
Comment #9
jhedstromThat would probably be the most flexible solution here.
Comment #10
tim.plunkettComment #11
nagy.balint CreditAttribution: nagy.balint commentedSo every module that needs these data needs to implement this alter and add the information into the element?
But if there are more than one module doing this in the system that seems to not be very efficient.
Is there a good reason why we would not add this basic information into the element by default like in the patch?
Comment #12
jhedstromThat's a good point. I don't see the harm in adding these details, but we'll still need a test.
Also, while we could make a public method
getFieldDefinition()
toWidgetBase
, it wouldn't be part of any sort of interface, so contrib modules that relied on its usage could only do so for widgets that extend that class.Comment #13
mr.york CreditAttribution: mr.york at Agence Inovae commentedComment #15
mr.york CreditAttribution: mr.york at Agence Inovae commentedComment #16
mr.york CreditAttribution: mr.york at Agence Inovae commentedWrote little tests.
Comment #17
alexpott@mr.york "Reviewed & tested by the community" means that the fix has been reviewed and tested by someone other than the patch author.
Comment #18
Aron NovakJust did a review, the code itself is fine, I have just a general feeling that the Widget-related things would deserve a separate test class, we modify Field\WidgetBase, and test it in field\Kernel\FieldAttachOtherTest , when for example there's field\Kernel\WidgetPluginManagerTest (sure, this does not belong there), but i think this is out of scope here, as this test already tests things related to widgets.
Comment #19
nagy.balint CreditAttribution: nagy.balint commented+1
This is blocking #2117827: "None" showing up as an option in multiselect. in the chosen module.
Comment #20
alexpottI don't think this is really a feature - just a task - and it is blocking contrib. Given it's BC I wonder whether we should consider for 8.1.1?
Comment #21
alexpottAlso no one has given a good response to why #8 is not the way to go?
Comment #22
nagy.balint CreditAttribution: nagy.balint commentedI thought #11 was a good reason, seeing that this patch only adds 2 lines, that probably should be available anyways without having to do all sorts of workarounds to get the data.
Of course that function could be available as described in #8 but then maybe it could be done in a separate issue, and could be used for some other more complex requirement.
Comment #23
mr.york CreditAttribution: mr.york at Agence Inovae commentedComment #24
jhedstromI agree #11 is a pretty good reason. Also, I mentioned that a method on a base class could not be relied on the same way as an interface class in #12. Contrib could only get information on widgets directly extending from the base class, but if a widget didn't use that base class, the method would not reliably be there.
Could we add the method to
WidgetBase
andWidgetInterface
in a point release, or is that considered an API change?Comment #25
star-szrThis looks to me like a reasonable solution and unless I'm missing something the pattern is something we use elsewhere (unfortunately) when we're not dealing with real objects. In this case a render array. Often it's hard to determine context and from what I'm seeing this is contextual information that is probably useful in many cases. I think the hypothetical OO render API would be helpful here if we wanted to keep it cleaner. Luckily the amount of data is quite small here so I don't think it's too concerning.
Comment #26
alexpottSo I think it would be possible to add getFieldDefinition to WidgetBaseInterface in 8.2.x - in fact that interface has the following documentation:
Which seems pretty analogous to what we want to do here. If someone can point to a widget in the wild not extending WidgetBase then we can explored the solution in #15. That solution is problematic because what happens when we want to do add the next thing from the field definition to the render element.
Comment #27
alexpottAfter discussing this more with @catch, I think we should try to avoid less #random in render arrays - see #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) for why. Yes this is a long way off - but we have a better way available here and one that means everything from the field definition is available - not just what we in core deem necessary.
Comment #28
jhedstromI'm going to work on the interface change.
Comment #29
jhedstromI went to add this method to the interface, but then when looking into tests, I found this in the
field_test_field_widget_form_alter()
implementation:Looking into
WidgetBase::formSingleElement()
, the$context['items']
implementsFieldItemListInterface
, which definitely has access to the field definition.So, this may be works as designed?
Comment #34
jhedstromClosing out.