Updated: Comment #N
Problem/Motivation
When you specify a field type that doesn't exist, due to some weird Typed Data magic, it still returns a generic list instance, which then results in a fatal error.
Proposed resolution
Check the type in advance and throw a helpful exception.
Remaining tasks
User interface changes
API changes
Original report by @ivanjaros
I have created a custom entity and I have defined it's fields in baseFieldDefinitions() method.
But if I use non-core type(Drupal\Core\Field\Plugin\Field\FieldType), like 'decimal' which is provided by Number module(Drupal\number\Plugin\Field\FieldType\DecimalItem) I'll end up with error. So I had to change the type to float, which is a "core" type, for time being.
Here is what happened when I've used non-core type #2164961: Call to undefined method setLangcode() when creating a field.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | d8_field_type_error_message-2165223-7.patch | 797 bytes | berdir |
| #5 | d8_field_type_error_message.patch | 797 bytes | fago |
| #3 | field-type-2165223-3.patch | 921 bytes | berdir |
Comments
Comment #1
berdirWow, this is strange, let's see what @fago has to say about this, I don't see a reason why this wouldn't work, looks like something with the list class is messed up?
Comment #2
berdirOk, found some time to debug this, the reason is quite simple.
This happens if the field type is not existing. The typed data manager first uses the data type 'list', which is ItemList and that is then overriden by the FieldDefinition, but only if the referencing field type is exposed as a data type.
This needs better error handling somewhere, but that's the only problem here.
Comment #3
berdirHere's a patch with some basic validation. We could also validate this when building the field definitions in the entity manager?
Also, the way the list overrides work is really hard to debug, wouldn't it be better to use an actual type for that? Then we might notice this earlier rather than returning a bogus data instance?
Comment #4
berdirUpdating title and issue summary.
Comment #5
fagoYeah, I think we should do the check when instantiating the object somewhere. I've not tested it, but attached patch should let it throw a better error message while instantiating the field object.
Related, this code did not through any notice because of #2220259: DefaultPluginManager::getDefinition() does not match the interface.
Comment #6
fagoComment #7
berdirThanks, manually verified. I don't thin this needs tests, this is a like calling a function when that module is disabled or a wrong schema definition, but we want to be a nicer to the developer and tell him where the problem is.
To reproduce:
Make sure you have a node.
Go to Node::baseFieldDefinitions() and change for example the title field type (string) to bla.
$ drush cr
HEAD: frontpage
Notice: Undefined index: list_class in Drupal\Core\TypedData\ListDataDefinition->getClass()
InvalidArgumentException: Cannot set a list with a non-array value. in Drupal\Core\TypedData\Plugin\DataType\ItemList->setValue()
And when trying to add new content on node/add/article, you get this:
Fatal error: Call to undefined method Drupal\Core\TypedData\Plugin\DataType\ItemList::setLangcode() in .../core/lib/Drupal/Core/Entity/ContentEntityBase.php on line 430
=> Not helpful.
With this patch:
LogicException: An invalid data type field_item:bla has been specified for list items. in Drupal\Core\TypedData\ListDataDefinition->getClass()
then you at least know that there's a bla thingy somewhere that you need to fix. It's still typed data'ified but maybe that will change in the typed data adapter issue.
Just changing %plugin_id to @plugin_id because it's double escaped otherwise.
Comment #8
alexpottCommitted ebfc1a4 and pushed to 8.x. Thanks!