Some of the base field overrides that were introduced in #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields have wrong names and storage types. For example the "default_value_function" is used instead of "default_value_callback", and it is set to the array ['Drupal\node\Entity\Node', 'getCurrentUserId'] where it should be the string 'Drupal\node\Entity\Node::getCurrentUserId'.

This bug has been exposed by looking up the user reference in seven_form_node_form_alter().

Steps to reproduce:

- Clean install with either the standard profile or the minimal profile with the Seven theme enabled.
- Enable Language and Content Translation modules.
- Add a new language to your site. Any one will do.
- Go to admin/config/regional/content-language, check Content, check Article. Save.
- Go to node/add/page.

This should produce a fatal error:
Call to a member function getUsername() on a non-object in /path/to/your/installation/core/themes/seven/seven.theme on line 229

Interestingly, checking both Article and Basic page, saving and then unchecking both will lead to both node types causing the error when you try to add them. Setting this one to critical because of white page of death.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

With help from @GaborHojtsy we discovered that this is due to base field overrides not being applied correctly to the user reference in the node entity. Base field overrides have been recently introduced in issue #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields.

When the content translation module is enabled it will override several node properties. For some reason the "default_value_callback" for the "uid" property is not overridden correctly for non-translated entities and is empty. It should normally point to Node::getCurrentUserId().

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Issue summary: View changes
Berdir’s picture

Status: Active » Needs review
FileSize
16.71 KB

This should fix it, can you try?

You need to start fresh without existing field overrides.

Berdir’s picture

Issue tags: +Needs tests
pfrenssen’s picture

Assigned: Unassigned » pfrenssen

I'll test it.

Berdir’s picture

Quickly discussed the testing with @Gabor.

I think we can focus on the actual bug here and don't need to involve content_translation/seven.

Create a new kernel test, probably in node.module, or extend one of the related tests there, then in there, do this:

1. Get the field definition for the uid field from the entity manager. Call getConfig()->save() on that, that should save the field override.
2. Create a new node
3. Check that getOwner() returns a user object (the anonymous user)

Additionally, we should check the new validation in setDefaultValueCallback() by extending BaseFieldDefinitionTest, which currently has no test coverage for that method. Add..

1. A test method that passes in a string and makes sure that works
2. A test that passes in an array and makes sure that throws the expected exception.

pfrenssen’s picture

Did a manual test. It works well now, thanks!! Will write a test.

pfrenssen’s picture

Title: Fatal error on node add page with certain Content Translation settings » Base field overrides have mismatched name and type
Issue summary: View changes
alexpott’s picture

Issue tags: +beta blocker

This blocks the beta since is changing the Entity Field API and configuration.

vijaycs85’s picture

FileSize
16.83 KB
550 bytes
+++ b/core/config/schema/core.data_types.schema.yml
@@ -364,7 +364,7 @@ field_config_base:
       label: 'Default value function'

Minor, but the label need to be updated.

larowlan’s picture

Assigned: pfrenssen » larowlan

takes on test

Status: Needs review » Needs work

The last submitted patch, 11: 2346911-default-value-callback-11.patch, failed testing.

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
2.02 KB
18.7 KB

The last submitted patch, 14: 2346911-default-value-callback.fail_.patch, failed testing.

plach’s picture

Component: content_translation.module » field system

Moving to the right queue :)

larowlan’s picture

here's the phpunit coverage requested at #7

Also fixed up the coverage annotations, here's the report - not great reading for such a high change-risk-anti-pattern index score.

larowlan’s picture

larowlan’s picture

@berdir pointed out that default values don't mean biscuit if you call save(), removed that from the new test.

here's fail/pass again.

If this is red/green I think its rtbc

The last submitted patch, 19: 2346911-default-value-callback.fail_.patch, failed testing.

larowlan’s picture

red/green - and with the right fails this time

tim.plunkett’s picture

  1. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -364,9 +364,9 @@ field_config_base:
    -    default_value_function:
    +    default_value_callback:
    

    There's a lot of this. That's a fine change for clarity, but I don't see how it's related to the issue title.

  2. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
    @@ -411,6 +411,9 @@ public function getDefaultValue(ContentEntityInterface $entity) {
       public function setDefaultValueCallback($callback) {
    +    if (!is_string($callback)) {
    +      throw new \InvalidArgumentException('Default value callback must be a string, like "function_name" or "ClassName::methodName"');
    +    }
    

    We can typehint Callable here, that should be enough, right?

  3. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -334,7 +334,7 @@ public function isRequired() {
    +    if ($function = $this->default_value_callback) {
    

    @chx points out this should be $callback

  4. +++ b/core/modules/node/src/Tests/NodeFieldOverridesTest.php
    @@ -0,0 +1,69 @@
    +<?php
    +/**
    

    Missing blank line

  5. +++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
    @@ -190,6 +219,9 @@ public function testFieldCardinality() {
    +   * @covers ::isRequired()
    +   * @covers ::setRequired()
    

    Not worth a reroll, but please don't use parens in @covers. We can fix it in #2328919: Remove () from a bunch of @covers definitions in PHPUnit

larowlan’s picture

Title: Base field overrides have mismatched name and type » Base field overrides have incorrect default value callable properties
FileSize
253.41 KB
2.53 KB
24.58 KB

#1 new title
#2 config schema only supports string, not array so can't use any callable, only string ones - but docblock says supports NULL and that breaks the is_string, so fixed in attached
#3-5 fixed

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Shame about Callable, but that makes sense.

Berdir’s picture

Yes, what we are validating is that we have the subset of callable that is compatible with being saved in configuration. Changes look good to me, thanks @larowlan.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5fdcfc7 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/node/src/Tests/NodeFieldOverridesTest.php b/core/modules/node/src/Tests/NodeFieldOverridesTest.php
index 8765353..b5d13b4 100644
--- a/core/modules/node/src/Tests/NodeFieldOverridesTest.php
+++ b/core/modules/node/src/Tests/NodeFieldOverridesTest.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\node\Tests;
 
-use Drupal\Component\Utility\String;
 use Drupal\Core\Field\Entity\BaseFieldOverride;
 use Drupal\node\Entity\Node;
 use Drupal\node\Entity\NodeType;
diff --git a/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
index 98fd22e..17cb518 100644
--- a/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
@@ -297,8 +297,8 @@ public function testNullDefaultValueCallback() {
    *
    * @param \Drupal\Core\Entity\EntityInterface $entity
    *   Entity interface.
-   * @param \Drupal\Core\Field\BaseFieldDefinitionInterface $definition
-   *   Base field definition.
+   * @param \Drupal\Core\Field\FieldDefinitionInterface $definition
+   *   Field definition.
    *
    * @return string
    *   Default value.

Fixed on commit - BaseFieldDefinitionInterface does not exist - the common interface for both BaseFieldDefinition and FieldConfigBase is FieldDefintionInterface

  • alexpott committed 5fdcfc7 on 8.0.x
    Issue #2346911 by larowlan, vijaycs85, Berdir | sihv: Fixed Base field...

Status: Fixed » Closed (fixed)

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