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.
Problem
- When adding a path field in the UI, then you get the following error message and the field is not added to the entity:
Notice: Undefined index: columns in Drupal\Core\Field\FieldDefinition->getSchema()
(line 484 of core/lib/Drupal/Core/Field/FieldDefinition.php).
Cause
- Computed fields do not have a schema.
- The problem is that getSchema() should initialize columns to an empty array by default, but does not.
Comment | File | Size | Author |
---|---|---|---|
#14 | 2257769-14-field-definition-empty-columns.patch | 8.65 KB | tstoeckler |
#4 | 2257769-3-field-definition-columns--test-only.patch | 7.82 KB | tstoeckler |
Comments
Comment #1
lokapujyaWould someone be kind enough to define "computed field"?
pathItem schema was added in this issue: #2144327: Make all field types provide a schema() and returns an empty array.
Comment #2
tstoecklerHmm... I'm certain that the bug was exposed in some way by #2201051: Convert path.module form alters to a field widget, because after that #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities started throwing notices, but I'm not sure right now why that is.
Anyway, working on tests right now. I had a fix already, which wasn't brought over here, though :-/
Comment #3
sun@tstoeckler: Thanks. And yes, I didn't want to get credited for your patch/fix by moving it over here. (I did play with the idea of [ab]using my d.o powers to attach it in a comment + change the author to be you afterwards, but didn't want to do that ;))
Comment #4
tstoecklerHere we go. Here's a tests-only patch that should fail and one with the fix.
I added a FieldDefinitionTestCase that acts as a base class for integration tests with FieldDefinition and particular FieldType plugins.
I also enabled 'path' module in an entity reference web test to act as a regression test.
Since there are currently no unit tests for Path module I chose to create them in the PSR-4 directory layout. There's no point in creating classes now just to move them later, IMO.
EDIT: Heh, I didn't realize we were taking commit credit that seriously. http://ericduran.github.io/drupalcores/ Everyone must assume that you've been cheating anyway, @sun... ;-P (kidding, of course!!!)
Comment #6
sunCan't we derive the module name from the plugin namespace (key) using
basename()
and remove this method?Returning an
ArrayObject
here looks a bit odd... can't we move that conversion intosetUp()
?Also, do we really need multiple namespaces?
Missing leading \, AFAIK.
That's unnecessary, performed by UnitTestCase already.
Comment #7
tstoecklerThanks for the review!
Your suggestions make sense. Not requiring to specify the module name required a little bit of trickery, let's see if you like it :-)
Comment #8
BerdirI'm rather sure there is a unit test for FieldDefinition (I wrote it myself ;)), so this should extend that? Also we name tests ..Test, not TestCase. (I know the base class is named differently, but I already pointed it out there and was ignored, but #2216557: Fill in @defgroup/topic docs for Testing overview will make this official ;)
Should also follow the @covers stuff we do now...
Comment #9
tstoecklerRe #8: I disagree. FieldDefinitionTest is a true unit test, i.e. it uses a mocked FieldTypePluginManager to test that FieldDefinition itself works correctly. This is an integration test which actually uses AnnotatedClassDiscovery to parse the actual annotation on the actual class. And FieldDefinitionTestCase is a helper base class for that. So I don't think it should extend FieldDefinitionTest. It couldn't re-use any of the methods.
Regarding the naming I chose the "Case" suffix because we currently have "UnitTestCase" which is the base class for all PhpUnit tests. I don't really care, but I think it would be confusing to name it just *Test, as it's not an actual test.
Regarding the @covers stuff, you mean @coversDefaultClass? As this covers both methods in FieldDefinition and in PathItem (again, it's an integration test) I don't really think that applies here. We *could* choose one of them as the default class, but I don't know which one would be a sensible choice.
Leaving at needs review to get some more eyes on this.
Comment #10
BerdirIf it's a base class, then the standard suffix would be TestBase :) I missed that it's an abstract class, sorry...
Comment #11
BerdirAnd thanks for fixing the issue title, "adding a path field" got me confused, as you shouldn't be able to do that :)
Comment #12
tstoecklerYeah, I seriously confused myself, because the problem arises when adding an entity reference field, but the problem that causes the bug is in PathItem...
Anyway, here's a patch that renames the test class to FieldDefinitionTestBase. I'm not really sure there is a standard at all currently, TBH, as we have both UnitTestBase and UnitTestCase. But anyway...
Comment #13
andypostno reason in extra lines
Comment #14
tstoecklerSure. Anyone want to RTBC?
Comment #15
lokapujyaVerified that I can create an entity reference without a notice.
Comment #17
webchickGood stuff, thanks!
Committed and pushed to 8.x.
Comment #18
sunTo address some of the discussion aspects above:
#2260121: PHPUnit Tests namespace of modules is ambiguous with regular runtime namespace (+ Simpletest tests)
I agree we need to improve that situation and quite possibly rename and/or move some classes elsewhere; it is very confusing right now.
Comment #20
donquixote CreditAttribution: donquixote commentedEven though the patch attempts to work with PSR-0 and PSR-4, the following breaks in #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4:
See also
FieldDefinitionTestBase::setUp()
:There are different ways to fix this. Any thoughts?
Comment #21
donquixote CreditAttribution: donquixote commentedFrom #2247991-34: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4
Comment #22
adammaloneWorking through this with donquixiote today on IRC the issue was that with PSR-4 structured modules, getNamespacePath needs to provide a module name and a path to the module directory.
We discussed three different implementations that could allow this to work (and would allow #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 to pass tests), although one thing we were unsure about is what the best/recommended approach would be.
I prefer the first idea simply for neatness reasons although there could well be a better way.
Comment #23
donquixote CreditAttribution: donquixote commentedFollow-up: #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4