Problem/Motivation

\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::onDependencyRemoval() sets $handler_settings['auto_create'] = NULL; but auto_create is defined as a Boolean in config schema - see entity_reference_selection.default in core/config/schema/core.data_types.schema.yml

Discovered as part of #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests

Proposed resolution

Change the code to set it to FALSE

Remaining tasks

Determine why schema is not being enforced on config save. see #3.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#9 3082289-9.patch1.78 KBKrzysztof Domański
#9 3082289-9-test-only.patch879 bytesKrzysztof Domański
#9 interdiff-2-9.txt1.54 KBKrzysztof Domański
#2 3082289-2.patch3.35 KBalexpott
#2 3082289-2-test-only.patch2.42 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3082289-2-test-only.patch2.42 KB
3082289-2.patch3.35 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented

So the reason that this is not casted to a Boolean is that we have special logic around null values \Drupal\Core\Config\StorableConfigBase::castValue() in specifically:

    if (is_scalar($value) || $value === NULL) {
      if ($element && $element instanceof PrimitiveInterface) {
        // Special handling for integers and floats since the configuration
        // system is primarily concerned with saving values from the Form API
        // we have to special case the meaning of an empty string for numeric
        // types. In PHP this would be casted to a 0 but for the purposes of
        // configuration we need to treat this as a NULL.
        $empty_value = $value === '' && ($element instanceof IntegerInterface || $element instanceof FloatInterface);

        if ($value === NULL || $empty_value) {
          $value = NULL;
        }
        else {
          $value = $element->getCastedValue();
        }
      }
    }

So we leave nulls alone. This is a much much wider issue if we want to fix that. I think the current small fix is okay but I'll check for follow-ups about this wider issue of config and nulls. Config schema does have the concept of nullable but funnily enough this is only really supported by sequences and mapping types - even if there places in core where people have added it to scalars.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Issue summary: View changes

The last submitted patch, 2: 3082289-2-test-only.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented 
+++ b/core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAdminTest.php
@@ -335,7 +335,7 @@ public function testMultipleTargetBundles() {
-    $this->assertFalse($field_config->getSetting('handler_settings')['auto_create']);
+    $this->assertSame(FALSE, $field_config->getSetting('handler_settings')['auto_create']);

+++ b/core/modules/user/tests/src/Functional/Views/AccessRoleTest.php
@@ -50,8 +50,8 @@ public function testAccessRole() {
-    $this->assertFalse($executable->display_handler->access($this->webUser));
-    $this->assertTrue($executable->display_handler->access($this->normalUser));
+    $this->assertSame(FALSE, $executable->display_handler->access($this->webUser));
+    $this->assertSame(TRUE, $executable->display_handler->access($this->normalUser));

Do we need @todos to revert this to assertTrue and assertFalse in #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests? Other than that, RTBC I'd say

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

#6 not necessary, will be done in a bulky way later.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Priority: Normal » Critical

Critical as it is holding the Critical parent

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
FileSize
interdiff-2-9.txt1.54 KB
3082289-9-test-only.patch879 bytes
3082289-9.patch1.78 KB

1. The patch contained code from another issue #3082287: \Drupal\user\Plugin\views\access\Role::access() does not conform to the base class documentation. I deleted it.

2. Follow-up for #6: #3082415: Replace assert(Not)Same/Identical() on booleans with assert(Not)True/False() in PHPUnit tests.

3. Needs follow-up for #3.

The last submitted patch, 9: 3082289-9-test-only.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented

whoops... @Krzysztof Domański thanks for tidying that up!

The last submitted patch, 9: 3082289-9-test-only.patch, failed testing. View results

  • catch committed 9af697c on 8.8.x 
    Issue #3082289 by Krzysztof Domański, alexpott: \Drupal\Core\Field\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 9af697c and pushed to 8.8.x. Thanks!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext commented
Issue tags: +DER issue

Status: Fixed » Closed (fixed)

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