Problem/Motivation
hook_widget_info_alter() is not called anymore.
Proposed resolution
Add AlterDecorator to the discovery object used by WidgetPluginManager.
Remaining tasks
Review the patch.
User interface changes
None.
API changes
None, really. hook_field_widget_info() wasn't called for a while, but that is really a regression, instead of this being an API change. Note that this is not marked critical or even major, even though it is a regression. That is because this patch is trivial enough that I don't fear we will ship without it. Feel free to elevate the priority if you disagree.
Comment | File | Size | Author |
---|---|---|---|
#16 | 1817180-16-widget-alter.patch | 3.92 KB | nils.destoop |
#13 | 1817180-13-widget-alter-test.patch | 2.41 KB | nils.destoop |
#13 | 1817180-13-widget-alter.patch | 3.63 KB | nils.destoop |
#10 | 1817180-10-widget-alter-test.patch | 1.22 KB | nils.destoop |
#10 | 1817180-10-widget-alter.patch | 2.44 KB | nils.destoop |
Comments
Comment #1
tstoecklerHere we go.
Comment #2
tstoecklerNote for reviewers: $this->baseDiscovery was never actually documented as a member variable, and is not used anywhere else, so I took the liberty of removing it as such.
Comment #3
yched CreditAttribution: yched commentedIf I'm not mistaken, baseDiscovery is used in clearDefinitions(), where we reconstruct a new CacheDecorator object. baseDiscovery just allows not to rebuild the whole stack of decorators.
Comment #4
larowlanTagging
Comment #5
tim.plunkettThis should continue to use
$this->baseDiscovery
, see #1764232: CacheDecorator provides no way to clear cached definitionsOtherwise, it looks good.
Comment #6
amateescu CreditAttribution: amateescu commentedThis looks good to me as well. Reverted the
$this->baseDiscovery
change.Discussed the 'Needs tests' part a bit with @swentel on IRC and we agreed that we should create a dedicated issue for that, because there are other parts from Field API that needs to be tested as well (e.g. hook_field_formatter_info(_alter)) and let this bug fix get in sooner.
Comment #7
tim.plunkettRTBC from me as well.
Comment #8
webchickHm. Not sure I'm totally on board with committing a fix for this without tests; accidentally completely removing a hook is a pretty big deal... :\ OTOH, I guess I could see it from the standpoint of trying to get a more holistic set of tests for all of the various phases/alters of field API.
I guess it's okay, but before committing this, let's make sure we have that follow-up issue, and that it's marked at least major.
Comment #9
aspilicious CreditAttribution: aspilicious commentedThis isn't blocking something. Lets write a small test. It just should check the alter is called.
Comment #10
nils.destoop CreditAttribution: nils.destoop commentedI added a WidgetTest class where all widget tests can be added. The alter test is already included.
First patch: new tests only
Second patch: bug fix + new tests
Comment #11
tstoecklerThe new standard is to do \Exception in-line. It doesn't seem to be used here, though.
Might be safer to create a small test module rather than testing the actual core modules. Don't know if that's a show stopper, though, as the test correctly shows that the hook is invoked.
Comment #12
yched CreditAttribution: yched commentedThanks @zuuperman :-)
For the test, though I'd rather suggest:
- add a test class in FieldInfoTest.php, that's where available widgets and formatters are currently tested.
- use one of the test_field widgets, rather than relying on the 'real' options widgets being used for taxo fields (which might change, for instance if taxo fields get ditched in favor of entity_reference at some point).
For instance 'test_field_widget_multiple': leave an empty 'field_types' entry in the annotation in TestFieldWidgetMultiple, and add it back in a hook_field_widget_info_alter() implementation in field_test.field.inc ?
For consistency, I guess we should do the same with one of the test formatters...
Comment #13
nils.destoop CreditAttribution: nils.destoop commentedRemoved the testclass, and added testWidgetDefinition to FieldInfoTest.php.
I implemented like yched proposed: remove field_types from TestFieldWidgetMultiple, and add it back with hook_field_widget_info_alter.
When changing this. I noticed that taxonomy module still uses 'field types' as key, but it should be 'field_types'. Gonna make separate issue for this.
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis completely breaks modules providing additional widgets to existing field types, so this is actually critical.
Comment #15
yched CreditAttribution: yched commented#13 looks good, would just need a couple comments for the test flow. Something like:
- in TestFieldWidgetMultiple.php's phpdoc :
"The 'field_types' entry is left empty, and is populated through hook_field_widget_info_alter().
@see field_test_field_widget_info_alter()"
- in testWidgetDefinition():
"The 'test_field_widget_multiple' widget is enabled for the 'test_field' field type in field_test_field_widget_info_alter()."
Also, I'd rather have the test use field_info_widget_types(), like the rest of the existing tests in this file, rather than the Plugin API directly.
Comment #16
nils.destoop CreditAttribution: nils.destoop commentedUpdated patch.
Comment #17
podarokcrazy code $)
Comment #18
aspilicious CreditAttribution: aspilicious commentedBut I love it ;)
This is good to go.
Comment #19
webchickI agree that looks a bit funky, but that's consistent with what we do elsewhere with alter decorators, afaik.
Great work, folks! Committed and pushed to 8.x.
It probably also makes sense to backport that test to 7.x, too, but that's just a normal task.