Problem

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

  1. Computed fields do not have a schema.
  2. The problem is that getSchema() should initialize columns to an empty array by default, but does not.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lokapujya’s picture

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

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Status: Active » Needs work

Hmm... 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 :-/

sun’s picture

@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 ;))

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
8.75 KB

Here 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!!!)

The last submitted patch, 4: 2257769-3-field-definition-columns--test-only.patch, failed testing.

sun’s picture

  1. +++ b/core/modules/path/tests/src/Field/PathFieldDefinitionTest.php
    @@ -0,0 +1,69 @@
    +  protected function getModuleName() {
    +    return 'path';
    +  }
    

    Can't we derive the module name from the plugin namespace (key) using basename() and remove this method?

  2. +++ b/core/modules/path/tests/src/Field/PathFieldDefinitionTest.php
    @@ -0,0 +1,69 @@
    +  protected function getPluginNamespaces() {
    +    return new \ArrayObject(array(
    +      'Drupal\path' => dirname(dirname(dirname(__DIR__))) . '/lib/Drupal/path',
    +    ));
    +  }
    

    Returning an ArrayObject here looks a bit odd... can't we move that conversion into setUp()?

    Also, do we really need multiple namespaces?

  3. +++ b/core/modules/path/tests/src/Field/PathFieldDefinitionTest.php
    @@ -0,0 +1,69 @@
    +   * @covers Drupal\Core\Field\FieldDefinition::getColumns
    +   * @covers Drupal\path\Plugin\Field\FieldType\PathItem::getSchema
    

    Missing leading \, AFAIK.

  4. +++ b/core/tests/Drupal/Tests/Core/Field/FieldDefinitionTestCase.php
    @@ -0,0 +1,88 @@
    +  public function tearDown() {
    +    // Remove any services from the global container.
    +    \Drupal::setContainer(new ContainerBuilder());
    +  }
    

    That's unnecessary, performed by UnitTestCase already.

tstoeckler’s picture

FileSize
3.77 KB
8.65 KB

Thanks 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 :-)

Berdir’s picture

+++ b/core/modules/path/tests/src/Field/PathFieldDefinitionTest.php
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Field/FieldDefinitionTestCase.php

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

tstoeckler’s picture

Title: Adding a Path field in the Field UI throws a PHP notice; fails to add field » Adding an Entity Reference field in the Field UI throws a PHP notice; fails to add field

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

Berdir’s picture

If it's a base class, then the standard suffix would be TestBase :) I missed that it's an abstract class, sorry...

Berdir’s picture

And thanks for fixing the issue title, "adding a path field" got me confused, as you shouldn't be able to do that :)

tstoeckler’s picture

FileSize
1.64 KB
8.65 KB

Yeah, 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...

andypost’s picture

+++ b/core/modules/path/tests/src/Field/PathFieldDefinitionTest.php
@@ -0,0 +1,60 @@
+class PathFieldDefinitionTest extends FieldDefinitionTestBase {
+
+
...
+
+
+  /**
+   * {@inheritdoc}

no reason in extra lines

tstoeckler’s picture

FileSize
535 bytes
8.65 KB

Sure. Anyone want to RTBC?

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Verified that I can create an entity reference without a notice.

  • Commit ac31f29 on 8.x by webchick:
    Issue #2257769 by tstoeckler | sun: Adding an Entity Reference field in...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Good stuff, thanks!

Committed and pushed to 8.x.

sun’s picture

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

Status: Fixed » Closed (fixed)

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

donquixote’s picture

Even 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:

  /**
   * {@inheritdoc}
   */
  protected function getNamespacePath() {
    return dirname(dirname(dirname(__DIR__))) . '/lib/Drupal/path';
  }

See also FieldDefinitionTestBase::setUp():

  public function setUp() {
    $namespace_path = $this->getNamespacePath();
    // Suppport both PSR-0 and PSR-4 directory layouts.
    $module_name = basename($namespace_path);
    if ($module_name == 'src') {
      $module_name = basename($module_name);
    }
    $namespaces = new \ArrayObject(array(
      'Drupal\\' . $module_name => $namespace_path,
    ));

There are different ways to fix this. Any thoughts?

donquixote’s picture

From #2247991-34: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4

The problem is we need both the module name and the module dir, or the directory for psr4/psr0.
Unfortunately, there is no reliable way to determine a module name from a PSR-4 directory returned with getNamespacePath().

Therefore I replaced this method with getModuleInfoFilePath(). An info file tells us about module name and module directory.
Then instead of this method telling us whether to look in the psr-0 or the psr-4 directory, we simply look in both - like the rest of all plugin discovery code. We can clean this up when PSR-0 support ends.
https://github.com/donquixote/drupal/commit/df501b4c1209fb1a015c2e7efbd9...

OPTIONAL: In a subsequent commit, I added a default implementation for getModuleInfoFilePath(), which assumes that the plugins to test are in the same module as the test class. This allows child classes to no not specify this at all.
https://github.com/donquixote/drupal/commit/f005a680de5bd6ed014b476c2ad1...

It all depends what other tests we want to add that inherit FieldDefinitionTestBase.

adammalone’s picture

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

  • Create a getModuleName method and have PathFieldDefinitionTest.php return 'path'
  • Amend the getNamespacePath method to return an array of module name and module directory
  • Find the .info.yml file and obtain module name and module directory from that

I prefer the first idea simply for neatness reasons although there could well be a better way.