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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs review
FileSize
1.44 KB

Here we go.

tstoeckler’s picture

Note 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.

yched’s picture

If 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.

larowlan’s picture

Category: task » bug
Issue tags: +Needs tests

Tagging

tim.plunkett’s picture

Status: Needs review » Needs work

This should continue to use $this->baseDiscovery, see #1764232: CacheDecorator provides no way to clear cached definitions

Otherwise, it looks good.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
1.22 KB

This 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.

tim.plunkett’s picture

RTBC from me as well.

webchick’s picture

Hm. 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.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

This isn't blocking something. Lets write a small test. It just should check the alter is called.

nils.destoop’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
1.22 KB

I 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

tstoeckler’s picture

+++ b/core/modules/field/lib/Drupal/field/Tests/WidgetTest.php
@@ -0,0 +1,45 @@
+use Exception;

The new standard is to do \Exception in-line. It doesn't seem to be used here, though.

+++ b/core/modules/field/lib/Drupal/field/Tests/WidgetTest.php
@@ -0,0 +1,45 @@
+  public static $modules = array('options', 'taxonomy');

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.

yched’s picture

Thanks @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...

nils.destoop’s picture

Removed 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.

Damien Tournoud’s picture

Priority: Normal » Critical

This completely breaks modules providing additional widgets to existing field types, so this is actually critical.

yched’s picture

Status: Needs review » Needs work

#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.

nils.destoop’s picture

Status: Needs work » Needs review
FileSize
3.92 KB

Updated patch.

podarok’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.phpundefined
@@ -44,7 +45,7 @@ class WidgetPluginManager extends PluginManagerBase {
+    $this->baseDiscovery = new AlterDecorator(new WidgetLegacyDiscoveryDecorator(new AnnotatedClassDiscovery('field', 'widget')), 'field_widget_info');

crazy code $)

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

But I love it ;)
This is good to go.

webchick’s picture

Title: Regression: hook_widget_info_alter() is not called anymore » Tests: hook_widget_info_alter() is not called anymore
Version: 8.x-dev » 7.x-dev
Category: bug » task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

I 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.

  • webchick committed c2874bb on 8.3.x
    Issue #1817180 by zuuperman, tstoeckler, amateescu: Fixed Tests:...

  • webchick committed c2874bb on 8.3.x
    Issue #1817180 by zuuperman, tstoeckler, amateescu: Fixed Tests:...

  • webchick committed c2874bb on 8.4.x
    Issue #1817180 by zuuperman, tstoeckler, amateescu: Fixed Tests:...

  • webchick committed c2874bb on 8.4.x
    Issue #1817180 by zuuperman, tstoeckler, amateescu: Fixed Tests:...