Boolean field with #access FALSE cause EntityStorageException

That sounds like some default value is not applied? I guess your field doesn't have a default value, so maybe the NULL is passed along?

I've done a litte bit of debugging and found a difference regarding the values in $form_state supplied to the submit handlers:

- with '#access' set to true, $form_state->values['field_test']['value'] contains '0'
- with '#access' set to false, $form_state->values['field_test']['value'] contains 'false'

Looking at the form array, there's a difference, too:

- with '#access' set to true, '#default_value' contains 'false' and #value contains '0'
- with '#access' set to false, '#default_value' contains 'false' and #value contains 'false'

But in both cases, I don't get any errors.

Maybe it helps somehow.

I accidentally created a duplicate of this: #2645316: "Incorrect integer value" for boolean fields with disabled access Reposting my patch from there. As stated there, "SqlContentEntityStorage uses the schema from the field definition to cast the value, but only for base fields (or more correctly, fields that live in the share table). I guess it should be done also for non-base fields."

We'll definitely want a test for this

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1244,7 +1244,8 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
-            $record[$column_name] = !empty($attributes['serialize']) ? serialize($item->$column) : $item->$column;
+            $value = drupal_schema_get_field_value($attributes, $item->$column);
+            $record[$column_name] = !empty($attributes['serialize']) ? serialize($value) : $value;

The addition of a procedural function here caught my eye, but this class is also the only other place the function is used. So not in scope here to worry about that. Could be a followup.

The other usage has a comment above it that references #2279395: The PostgreSQL backend does not handle NULL values for serial fields gracefully; however, that other usage wasn't actually added in that patch. It was already there and has been at least since the EntityNG conversion.

Discussed with @xjm and @catch. We agreed that this is a major bug as this is something that it is reasonable to expected to work and currently there is no way to programmatically disable a boolean field on an entity without causing this error.

Same error occurs if you're setting the boolean field to FALSE programmatically:

$node->field_test->value = FALSE;
$node->set('field_test', FALSE);

I'm surprised this aspect has not been discovered and repaired before.

(IS edit: adding "Yes" to the patch complete cell)

Needs work for tests.

Ignore this one.

Ignore this one too.

Tried modifying a test to cover this situation.

1. +++ b/core/modules/field/tests/modules/deny_access/deny_access.module
   @@ -0,0 +1,12 @@
   +function deny_access_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   

   So, I created a module that sets the boolean field's #access to FALSE.

2. +++ b/core/modules/field/src/Tests/Boolean/BooleanFieldTest.php
   @@ -58,7 +58,7 @@ function testBooleanField() {
   +    $field_name = 'booleanmachinename';//Unicode::strtolower($this->randomMachineName());
   

   Had to use an an actual name for the field in order to do it.

@lokapujya: I don't think you should be modifying an existing test as that removes test coverage, try adding fields/cases to test instead

+++ b/core/modules/field/src/Tests/Boolean/BooleanFieldTest.php
@@ -58,7 +58,7 @@ function testBooleanField() {
+    $field_name = 'booleanmachinename';//Unicode::strtolower($this->randomMachineName());

Additionally, the resulting fails do not line up with the original error, perhaps because an existing test was modified:

exception: [Notice] Line 105 of core/modules/field/src/Tests/Boolean/BooleanFieldTest.php:
Undefined offset: 1Drupal\field\Tests\Boolean\BooleanFieldTest->testBooleanField() (Line: 1074)

That makes sense. This was just the quickest way to get started. Posted it to get the ball rolling. Is there a better place to put this test? Will wait for some more input before continuing. To anyone interested in contributing, feel free to go in a different direction if you want to post patch or add some more advice on creating this test.

Just keep the existing part of the test as it is, and instead duplicate the relevant parts, possibly into a new testFormAccess or so and only there use your hardcoded name. Then in the form alter, check if the field is present and deny access if so.

Instead of a form alter, you could also implement hook_entity_field_access().

hook_entity_field_access() returns the same fatal error.

drupal_schema_get_field_value is a really silly name it needs to be called (with a BC wrapper) drupal_schema_cast_field_value. Otherwise , great!

I tried to make new test case.

I believe you forgot to git add the deny_access module and as such it is missing from the patch. Also, as #21and #22 says you can use hook_entity_field_access as well instead of form alter. Hook form alter #access FALSE 'ing a field widget in an entity form is dubious at best but most likely is completely wrong and hook_entity_field_access should be used instead.

Just experienced this on a site. Working on this now, updating the patch per #25

OK, then. Was maybe a bit quick.

Here is a test-only patch and one with the suggested fix.

Great! thanks for the quick reroll. You can call ->getName on the fieldDefinition and use AccessResult::forbiddenIf for shorter code.

Did not run the test, but here is a change as suggested. should(tm) work :)

return AccessResult::forbiddenIf($field_definition->getName() === \Drupal::state()->get('field.test_boolean_field_access_field'));

forbiddenIf returns neutral if the condition is false and since we use === in the condition we know no funny business (like "foo" == 0) can ensue.

I posted that right before I went to bed, and realised that had to be what you meant. Almost got back up to post a new one :)

Both tests and the fix look good to me.

Committed f4da728 and pushed to 8.3.x. Thanks!

Setting as 'patch to be ported' for cherry-pick to 8.2.x once 8.2.0 is out.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1257,7 +1257,9 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
-            $record[$column_name] = !empty($attributes['serialize']) ? serialize($item->$column) : $item->$column;
+            $value = drupal_schema_get_field_value($attributes, $item->$column);
+            $record[$column_name] = !empty($attributes['serialize'])
+              ? serialize($value) : $value;

Wow - how painful that this database abstraction bleeds out into the entity storage layer. Funnily enough the only other usage of drupal_schema_get_field_value() is in SqlContentEntityStorage. It is tempting to ask for a followup issue to copy the logic into a castValue() method on this class and deprecate drupal_schema_get_field_value(). Doing that is certainly not in scope here.

diff --git a/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module b/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module
index 9628d43..be56c37 100644
--- a/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module
+++ b/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module
@@ -1,10 +1,20 @@
 <?php
+
+/**
+ * @file
+ * Helper module for the boolean field tests.
+ *
+ * @see \Drupal\field\Tests\Boolean\BooleanFieldTest::testFormAccess()
+ */
+
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Field\FieldDefinitionInterface;
+use Drupal\Core\Field\FieldItemListInterface;
+use Drupal\Core\Session\AccountInterface;
 
 /**
  * Implements hook_entity_field_access().
  */
-function field_test_boolean_access_denied_entity_field_access($operation, FieldDefinitionInterface $field_definition, \Drupal\Core\Session\AccountInterface $account, \Drupal\Core\Field\FieldItemListInterface $items = NULL) {
+function field_test_boolean_access_denied_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
   return AccessResult::forbiddenIf($field_definition->getName() === \Drupal::state()->get('field.test_boolean_field_access_field'));
 }

Coding standards fixes on commit.

Committed e0fd432 and pushed to 8.2.x. Thanks!

This broke something in Drupal commerce. The serialization needs to occur before the call to drupal_schema_get_field_value() just as in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord(). And we need to add a test for this.

The reason why this commit broke things is because it calls drupal_schema_get_field_value() before serialization, which attempts to cast an object to a string.

Possibly related: #2788023: Serialized single-value base field columns are not unserialized when loading entities and #2788637: Values in shared table for SQL content entity storage do not get unserialized..

This appears to still be an issue in 8.2.2

Added serialization before drupal_schema_get_field_value() is called.

@bojanz: Do we have something in core to be able to test this? Do we need a field type?

Added test to assert the serialization.
4 Files in attachment:
1. Test-only with the old patch
2. Interdiff between old patch and new (serialization made before drupal_schema_get_field())
3. Interdiff (the added test for the new patch)
4. Final working patch with test

Nice, just two tiny things and we should be good to go.

1. +++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestObjectItem.php
   @@ -0,0 +1,61 @@
   + *   description = @Translation("Dummy object field type used for tests."),
   

   copied I think?

   Lets write "Test field type that has an object as value to test serialization." as the description.

2. +++ b/core/modules/field/tests/src/Kernel/TestObjectItemTest.php
   @@ -0,0 +1,52 @@
   +
   +  public function testTestObjectItem() {
   

   missing docblock

Note that this actually also gives us test coverage for #2788637: Values in shared table for SQL content entity storage do not get unserialized., we just need to remove the @todo there.

Added description and missing docs for testObjectItem() function.

Thank you it works!!

Great work, I think this is ready to be committed again and hopefully stay in this time :)

See #51 for failing test.

Committed and pushed 44227b1 to 8.3.x and 9d41d1c to 8.2.x. Thanks!

diff --git a/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestObjectItem.php b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestObjectItem.php
index b8050d5..a20265e 100644
--- a/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestObjectItem.php
+++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestObjectItem.php
@@ -58,4 +58,5 @@ public function setValue($values, $notify = TRUE) {
     }
     parent::setValue($values, $notify);
   }
+
 }
diff --git a/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module b/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module
index 9628d43..e1ae2f7 100644
--- a/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module
+++ b/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module
@@ -1,10 +1,18 @@
 <?php
+
+/**
+ * @file
+ * Module for testing denying access to boolean fields.
+ */
+
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Field\FieldDefinitionInterface;
+use Drupal\Core\Field\FieldItemListInterface;
+use Drupal\Core\Session\AccountInterface;
 
 /**
  * Implements hook_entity_field_access().
  */
-function field_test_boolean_access_denied_entity_field_access($operation, FieldDefinitionInterface $field_definition, \Drupal\Core\Session\AccountInterface $account, \Drupal\Core\Field\FieldItemListInterface $items = NULL) {
+function field_test_boolean_access_denied_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
   return AccessResult::forbiddenIf($field_definition->getName() === \Drupal::state()->get('field.test_boolean_field_access_field'));
 }
diff --git a/core/modules/field/tests/src/Kernel/TestObjectItemTest.php b/core/modules/field/tests/src/Kernel/TestObjectItemTest.php
index 7443fa6..8a78755 100644
--- a/core/modules/field/tests/src/Kernel/TestObjectItemTest.php
+++ b/core/modules/field/tests/src/Kernel/TestObjectItemTest.php
@@ -20,6 +20,9 @@ class TestObjectItemTest extends FieldKernelTestBase {
    */
   public static $modules = array('field_test');
 
+  /**
+   * {@inheritdoc}
+   */
   protected function setUp() {
     parent::setUp();
 
@@ -52,4 +55,5 @@ public function testTestObjectItem() {
     $this->assertTrue($entity->field_test->value instanceof \stdClass);
     $this->assertEquals($object, $entity->field_test->value);
   }
+
 }

Coding standards fixes on commit.