plugin-fatal.png

That's all I see when trying to access /node/add.

Dumping $plugin_id and $plugin_definition yields NULL and NULL.

Such errors should not be possible.

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Component: plugin system » field system

A debug_backtrace() reveals that Field API's Widget Plugin Factory tries to instantiate a widget for this URL alias field instance:

Drupal\field\FieldInstance Object
(
    [definition] => Array
        (
            [label] => URL alias
            [widget] => Array
                (
                    [weight] => -3
                    [type] => path_default
                    [module] => 
                    [active] => 0
                    [settings] => Array
                        (
                        )

                )

            [settings] => Array
                (
                    [pattern] => Array
                        (
                            [prefix] => article
                            [default] => 
                        )

                    [user_register_form] => 
                )

            [display] => Array
                (
                    [default] => Array
                        (
                            [label] => above
                            [type] => hidden
                            [settings] => Array
                                (
                                )

                            [weight] => 1
                        )

                )

            [required] => 0
            [description] => 
            [default_value] => 
            [id] => 3
            [field_id] => 3
            [field_name] => field_url_alias
            [entity_type] => node
            [bundle] => article
            [deleted] => 0
        )

    [widget:protected] => 
    [formatters:protected] => 
)
yched’s picture

Mh, I guess the WidgetFactory is at fault here, should provide a better message :-/.

Just for context, can you tell me :
what is the 'path_default' widget type ? where is it provided ?
What's the field type of field_url_alias ? what's its 'default_widget' ?

sun’s picture

The path_default widget comes from #1751210: Convert URL alias form element into a field and field widget

I surely have to amend + admit that this plugin only exists in one of my development git branches and not on 8.x, on which I encountered the error message.

In other words: The entire field widget plugin vanished and does not exist in the code-base.

What I don't get though is why the $plugin_id is NULL. As visible in #1, the field instance actually refers to a 'type' "path_default" - and if I get it right, then that's the widget's plugin ID.

So the widget factory should actually still know the plugin's ID?

But yet, the exception is thrown on a $plugin_id that is NULL...?

Also, the DefaultFactory code shows this:

  protected function getPluginClass($plugin_id) {
    $plugin_definition = $this->discovery->getDefinition($plugin_id);

$plugin_definition is NULL, too. So a second issue: Shouldn't the discovery throw an exception of its own when trying to retrieve a definition for a particular, given plugin ID that does not exist?

tstoeckler’s picture

Shouldn't the discovery throw an exception of its own when trying to retrieve a definition for a particular, given plugin ID that does not exist?

That would make checking whether a plugin exists a bit cumbersome, having to catch an exception instead of simply checking for !empty($definition). I think it would make more sense for getPluginClass() to throw an exception if $definition is NULL.

yched’s picture

Status: Active » Needs review
FileSize
2.36 KB

What I don't get though is why the $plugin_id is NULL. As visible in #1, the field instance actually refers to a 'type' "path_default" - and if I get it right, then that's the widget's plugin ID.

That's because when the requested plugin id is unknown, WidgetManager::getInstance() backs off to the 'default_widget' specified in the field type info. But in your case the field type had vanished too, so that 'default_widget' is NULL.

I surely have to amend + admit that this plugin only exists in one of my development git branches and not on 8.x, on which I encountered the error message.
In other words: The entire field widget plugin vanished and does not exist in the code-base.

Yeah. Field types simply vanishing from the code base are hell to deal with.

Fields and instances whose field type is not available should be marked "inactive".
But when the field type simply disappeared (e.g after a rm or a git checkout), the "active" state is refreshed on next drupal_flush_all_caches(), in field.module's hook_rebuild(). We cannot really do better than that - nor do we want to litter our runtime code with "is this active field really active ?" checks...

As far as the cryptic error message is involved, best I can offer is attached patch - done for formatters too for consistency.
Drawback: is it adds a couple func calls on each widget / formatter instanciation :-/

Feedback welcome on the most relevant Exception class to use, and on the actual message :-)

swentel’s picture

Drawback: is it adds a couple func calls on each widget / formatter instanciation :-/

My initial reaction to this patch really is (and will probably stay for a while): this is babysitting. I mean, it's not like a field plugin is going to disappear during a request right ? I understand it's hard to to track it and will only be 'stable' when a drupal_flush_all_caches() happens, but that applies to so many things in D8 at this point that I'm not convinced to add extra function calls here.

sun’s picture

Note that I'd be happy with a fix for this issue that makes the error message in the OP expose the actual plugin ID.

The problem is the error message that throws an error without knowing on what it throws an error.

That sounds weird and horrible, but that's exactly what the shotted error message depicts: An error on NULL. Which translates into:

I've no idea what you want from me, and I've no idea what I was asked to do, because all you gave to me is nothing, and so in turn, all I can give to you is nothing, and you know, You're Doing It Wrong™. My pleasure.

The rest is left for #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term) (since the plugin ID on its own does not give any kind of clue at all).

yched’s picture

I probably agree, actually
[edit: crosspost - that was "I agree with @swentel #6" :-)]
[re-edit: and doh, fixed the code snippet below, was just a straight copy/paste of the current patch...]

We could do this instead, no overhead :

     if (!$definition || !in_array($field['type'], $definition['field_types'])) {
       // Grab the default widget for the field type.
       $field_type_definition = field_info_field_types($field['type']);
       $type = $field_type_definition['default_formatter'];
+      if (empty($type)) {
+        throw new \RuntimeException(t('No valid formatter could be found for field type @type.', array('@type' => $field['type'])));
+      }
     }

Still arguably on the slippy side of "start adding runtime checks for broken setups we can't really pretend to support", though...

yched’s picture

@sun: note that the problem here is not actually that the widget is missing, but that the field type is missing :-)

yched’s picture

OK, let's do this then.
Will also catch cases of field types failing to provide a default_[formatter|widget].

The last submitted patch, field_plugins_exception-1864014-10.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, field_plugins_exception-1864014-10.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +DrupalWTF, +Plugin system
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks good to me.

sun’s picture

Status: Reviewed & tested by the community » Needs review

That said, I hope this does not conflict with these two issues?
#1751210: Convert URL alias form element into a field and field widget
#1825044: Turn contact form submissions into full-blown Contact Message entities, without storage

As mentioned in #1849724: Avoid critical Field API limitations, [EDIT] both issues are adding a field that only has a widget.

It looks like this code here runs very early when the widget/formatter plugin is retrieved from the instance and immediately bails out when there is no plugin.

So let me first test this change with both of those issues.

sun’s picture

Issue tags: -DrupalWTF, -Plugin system
sun’s picture

Issue summary: View changes

Updated issue summary.

AlbertLamme’s picture

In the patch they spoke about a directory of 'type' in the 'plugin' directory. I can only see a 'views' directory. Do i miss something?

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll
ankitgarg’s picture

Status: Needs work » Needs review
FileSize
975 bytes

No valid formatter Exception Added from patch.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll +Needs tests
+++ b/core/lib/Drupal/Core/Field/WidgetPluginManager.php
@@ -103,8 +103,9 @@ public function getInstance(array $options) {
+      if (empty($field_type_definition['default_widget'])) {    ¶
         return NULL;
+        throw new \RuntimeException(t('No valid widget could be found for field type @type.', array('@type' => $field_type)));

This exception is thrown after returning, thus the code would never be executed.

Also some trailing whitespace.

I wonder if it would be possible to write a test for this?

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Plugin system, -Needs tests
FileSize
4.01 KB

We recently introduced a test to verify field plugin definition integrity. This patch extends that with checks for default widgets and formatters. Apparently there are quite a few incorrect definitions, which means this patch is supposed to fail.

Also removing the plugin system tag, as this is issue is specific to Field API.

Status: Needs review » Needs work

The last submitted patch, 22: drupal_1864014_22.patch, failed testing.

Xano’s picture

Title: PluginException: The plugin () did not specify an instance class. » Field types specify non-existent default widgets and formatters

What the patch from #22 shows is that core contains several items for which we may indeed not want to provide widgets, such as CreatedItem and ChangedItem. Unless we change this, not providing a default widget/formatter or there not being any available widgets/formatters for a field type at all cannot really be classified an error and as such not justify exceptions being thrown.

Xano’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
3.75 KB
Xano’s picture

I just discussed this with @yched at the Drupal Dev Days and our conclusion was that widgets and formatters should not be required for field types. It is good practice to provide widgets and formatters for every field type in core and contrib, but we do not want to burden developers of client projects with having to write dummy widgets and formatters for their custom field types.

Xano queued 25: drupal_1864014_25.patch for re-testing.

Xano queued 25: drupal_1864014_25.patch for re-testing.

lauriii queued 25: drupal_1864014_25.patch for re-testing.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
heddn’s picture

Status: Reviewed & tested by the community » Needs review

re #26: There is not default widget or formatter for MapItem. And the documentation for that field is a little vague, considering I can't see where it is even used in core. Otherwise, #2552799: FieldType with no available widget causes Fatal error seems to have picked up adding a default plugin for all other field types.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Resetting status. Not sure why/how that got changed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: drupal_1864014_25.patch, failed testing.

lauriii’s picture

It seems like Drupal\views\Tests\Update\EntityViewsDataUpdateTest had 22 fails. At least locally this passes and this patch shouldn't have any effect for that.

Status: Needs work » Needs review

lauriii queued 25: drupal_1864014_25.patch for re-testing.

Xano’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #31 and #33.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Seems like a good thing to have test coverage of. Committed 968bc41 and pushed to 8.0.x. Thanks!

  • alexpott committed 968bc41 on 8.0.x
    Issue #1864014 by Xano, yched, ankitgarg, sun: Field types specify non-...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.