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.
Comment | File | Size | Author |
---|---|---|---|
#23 | 2346911-default-value-callback.23.patch | 24.58 KB | larowlan |
#23 | interdiff.txt | 2.53 KB | larowlan |
#23 | the-dude.gif | 253.41 KB | larowlan |
#19 | 2346911-default-value-callback.pass_.patch | 24.2 KB | larowlan |
#19 | 2346911-default-value-callback.fail_.patch | 7.58 KB | larowlan |
Comments
Comment #1
pfrenssenWith 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().
Comment #2
pfrenssenComment #3
pfrenssenComment #4
BerdirThis should fix it, can you try?
You need to start fresh without existing field overrides.
Comment #5
BerdirComment #6
pfrenssenI'll test it.
Comment #7
BerdirQuickly 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.
Comment #8
pfrenssenDid a manual test. It works well now, thanks!! Will write a test.
Comment #9
pfrenssenComment #10
alexpottThis blocks the beta since is changing the Entity Field API and configuration.
Comment #11
vijaycs85Minor, but the label need to be updated.
Comment #12
larowlantakes on test
Comment #14
larowlanComment #16
plachMoving to the right queue :)
Comment #17
larowlanhere'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.
Comment #18
larowlanComment #19
larowlan@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
Comment #21
larowlanred/green - and with the right fails this time
Comment #22
tim.plunkettThere'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.
We can typehint Callable here, that should be enough, right?
@chx points out this should be $callback
Missing blank line
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
Comment #23
larowlan#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
Comment #24
tim.plunkettThanks! Shame about Callable, but that makes sense.
Comment #25
BerdirYes, 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.
Comment #26
alexpottCommitted 5fdcfc7 and pushed to 8.0.x. Thanks!
Fixed on commit - BaseFieldDefinitionInterface does not exist - the common interface for both BaseFieldDefinition and FieldConfigBase is FieldDefintionInterface