Handle nullable serialized field columns

Patch for drupal version 9.3.19

The change looks good to me, and seems to fix the issue.
Applied in D9.4

The steps to reproduce in the issue summary are not accurate. We are currently testing Drupal core on PHP 8.1 and this does not occur. This error is likely something else that is wrong and it would be great to understand what that is rather that doing unnecessary string casts everywhere.

Set proper status

Comment #8 from @alexpott has not been addressed.

I'm seeing a similar warning when programmatically loading a profile, when the data column for a profile's record in the profile_revision table is NULL:

Deprecated function: unserialize(): Passing null to parameter #1 ($data) of type string is deprecated in Drupal\Core\Entity\Sql\SqlContentEntityStorage->mapFromStorageRecords()

See #3324465: [PHP 8.1] Deprecated function: unserialize(): Passing null to parameter #1 ($data) of type string is deprecated in Drupal\Core\Entity\Sql\SqlContentEntityStorage->mapFromStorageRecords() for more details (I filed a bug report for Profile module, but not sure if that was the right place).

The patch in #4 fixes the warning.

It seems the deprecation warning occurs when $row[$column_name] is null which can be the case for any null-able table columns related to fields that are tried to be loaded here.
In the attached xdebug screenshot the columns link_override__uri, link_override__title and link_override__options trigger the warning.
The handled columns are provided by TableMapping->getColumnNames(). This method provides all column names, so i guess a check for empty ones could be necessary.

Just updated the branch on the related issue fork. @p-neyen's solution maked sense to me and applied it to the method SqlContentEntityStorage->loadFromSharedTables()
Also @alexpott's concerns are addressed there.

It seems legit to me, updated status.

Sorry for the bad status update, i test against the last merge request and what was reported by @brunodbo at #13 is still happening.

Also the cast to string on unserialize function seems to works (#4).

This is happening on taxonomy terms with an empty description. It looks like checking for the value before unserializing is a good solution though I believe null coalescing operator is better than type casting because casting an array to string would output "Array" rather than throwing an error.

#19 solves issue.

I am wondering if this issue is Normal priority?
This kind of makes drupal 9.5 & 10 incompatible with >php8.1, at least partially, no one wants warnings all over their website.

PHP 8.1 is recommended.

https://www.drupal.org/docs/system-requirements/php-requirements

And just want to add, 'map' field type from core is affected, I am not sure if it is used in core...
But commerce order uses it.

Basically all fields with serialized property are affected

Comment #8 points out that the core is tested on PHP 8.1 and this error is not occurring. In that comment steps to reproduce the problem were asked for and they have not been supplied.

@akoe, can you expand more on #16 where you said that alexpott's concerns are addressed?

This needs steps to reproduce so we can duplicate the problem manually and in a test.

I am setting this back for those steps to be added.

As mentioned in #13:

Steps to reproduce:
1. Create new entity type
2. Install newly added entity type
3. Create at least one entity (id: 1)
4. Add map base field to created entity type:

    $fields['data'] = BaseFieldDefinition::create('map')
      ->setLabel(t('Data'))
      ->setDescription(t('A serialized array of additional data.'));

5. load entity
6. Warning will occur

I've tried adding:

->setInitialValue([])

but it does not help

This seems like we are fixing a symptom of a possible bigger issue.

But either way it will need a test case showing the issue.

Here is failing test.

In case #19 patch is applied, test passes.

I've just updated existing test, I am not sure if we should create new test case for this issue?

Here is combined patch od #27 and #19

So #27 proves recreation of error...
And it is fixed in #28

Will move this one on and see what the committers think.

Thanks for the steps to reproduce. I used code from the examples module to play with this and wasn't able to reproduce the error.

I do agree with #26 that this may be fixing the symptom and not the underlying problem. This also should have a better title, one that describes the action that the issue is fixing or improving.

Yes, something is wrong with initial value...
- In case new entity is added after field installation, value for map field is empty array "a:0:{}" (maybe we can change to that in default value?)
- Empty string does return false when unserialized but so does null and drupal 8 and 9 worked properly whole lifetime

So I guess either we use

$record->{$column_name} ?? '' ('a:0:{}')

Or we fix initial value and create update hook for populating values for backward compatibility.

Edit: also all fields are set to null when empty, so I think for consistency we should be able to set map fields to NULL

#28 seems to work for me on 9.5.5

Patch #28 resolved the issue, thank you DavorHorvacki!

I think the root cause of this is probably related to #2346019: Handle initial values when creating a new field storage definition - maybe map item doesn't set an initial value in the field storage definition?

It could also be #2883851: Add initial values support for configurable and multi-valued fields though which is unresolved.

If we're able to fix this by setting an initial value, then I think we should. If it's due to #2883851: Add initial values support for configurable and multi-valued fields , then we should add a comment with a @todo to the places adding the null check.

So for us this happens with a custom base field of type 'link', that is set to be optional.

function mymodule_entity_base_field_info(EntityTypeInterface $entity_type) {
  [..]
  $fields['my_link_field'] = BaseFieldDefinition::create('link')
    ->setLabel(t('My custom link field'))
    ->setRequired(FALSE)
    // I suppose all the below is irrelevant.
    ->setTranslatable(TRUE)
    ->setRevisionable(TRUE)
    ->setSettings([
      'link_type' => LinkItemInterface::LINK_EXTERNAL,
      'title' => DRUPAL_DISABLED,
    ])
    ->setDisplayOptions('form', [
      'type' => 'link_default',
      'weight' => 0,
    ])
    ->setDisplayConfigurable('view', TRUE)
    ->setDisplayConfigurable('form', TRUE);
  [..]
  return $fields;
}

The base field adds three columns to `node_field_data` and `node_field_revision` tables:
- my_link_field__uri: varchar, allows NULL
- my_link_field__title: varchar, allows NULL
- my_link_field__options: serialized, allows NULL

Checking the database, I find a bunch of rows where `my_link_field__options` has the value NULL, and other rows where the value is `N;`.
The `changed` column shows that all those with value NULL are old, and those with value `N;` are newer.

So this means that, when the base field is added, the value for that column is initialized as NULL.
But when updating or creating content later, the column is initialized with `N;` (serialized NULL).

So the steps to reproduce would be:
- Create a node, while no custom base fields have been declared yet.
- Declare a custom base field in a module.
- Create another node.
- Load each node (separately), compare the output.
- Also check the database.

Observed behavior:
- Column is NULL for the old node, but 'N;' for the new node.
- Loading the old node produces a warning.

@catch

If we're able to fix this by setting an initial value, then I think we should.

I think the initial value idea is incomplete at best.

If a database column allows NULL, then we should not allow this to cause a problem when loading the entity.

So we need to do one of two things:

1. Not allow NULL for serialized columns from optional fields. Only allow 'N;'. This also means we need to set an initial value, and update old records.
2. Check for NULL before calling unserialize(). This is what the patch in this issue does, and this is what restores pre-8.1 behavior.

Interestingly, `serialize(NULL)` returns FALSE instead of NULL. This is also the legacy behavior, and this is what the patch does.
However, we probably rather want NULL instead of FALSE.
To achieve that, we could use `unserialize($row[$column_name] ?? 'N;')` instead of `unserialize($row[$column_name] ?? '')`.

But we would need to investigate the side effects.

For map field type default value after field is installed is 'a:0:{}' not 'N;', but NULL could work anyway

The patch is #28 worked for me.

The patch is #28 worked for me in drupal 9.5.9

Patch #28 also worked for me, in Drupal 9.5.4

Test only!

I've added link field to test and separated it to new test file.

I won't be working on this issue further because I still think #19 is easiest all around solution, backward compatible.

Thanks!

#28 also works for me under 10.1.4

#44 appears to be a tests only patch that didn’t run. Could we get also a full patch

Here is combined patch.

I'll move this to Needs review but #36 stays unresolved.

Think to move this forward till #2883851: Add initial values support for configurable and multi-valued fields is to do what @catch suggested and added a todo pointing to #2883851: Add initial values support for configurable and multi-valued fields

In case this helps

I had this specific

Deprecated function: unserialize(): Passing null to parameter #1 ($data) of type string is deprecated in Drupal\Core\Entity\Sql\SqlContentEntityStorage->mapFromStorageRecords()

In my case, was related to Profile, maybe some update in the past didnt go through or is missing

Simply loading/saving the entity changed value of data from false to array and message disappeared

#48 works perfect on d10.1.4 and PHP8.1

For others' reference, a client had a link field that was triggering this error on six nodes. The status page had no errors about the field and the configuration status was clean.

Resaving the nodes didn't fix the problem. Nor did copying data via SQL from the field table to the revision table. Here's what did:

1. Resave the field settings.
2. Resave the field storage settings.
3. Each node lost the link field data so copy and paste the link data from another server.

And after resaving the field, the configuration status still had no changes between database and file.

I focused on mapFromStorageRecords since that was the error that I was encountering. In my case an old profile entity had the data column set to NULL in the database.

From #48:
$values[$id][$field_name][LanguageInterface::LANGCODE_DEFAULT] = !empty($column['serialize']) ? unserialize($record->{$column_name} ?? '') : $record->{$column_name};

With this unserialize will return FALSE when passing an empty string. The entity field then ends up having a field item with value FALSE - which is wrong; the field item list should be empty.

I think the following would be better:

if (empty($column['serialize']) || isset($record->{$column_name})) {
  $values[$id][$field_name][LanguageInterface::LANGCODE_DEFAULT] = !empty($column['serialize']) ? unserialize($record->{$column_name}) : $record->{$column_name};
}
else {
  $values[$id][$field_name][LanguageInterface::LANGCODE_DEFAULT] = NULL;
}

@krystalcode could you updte patch so we can test.

In general is there a need to develop tests for this?

Started new merge request targeting 11.x with:

1. patch from #48
2. suggestion from #54
3. and links to #2883851: Add initial values support for configurable and multi-valued fields from #36

Good to see progress here! FWIW I just deployed the patch in #48 to a production site and it resolved the deprecation warnings described in https://www.drupal.org/project/profile/issues/3324465. I was a little surprised at the substantial CPU performance improvement for the site, presumably because these were invalidating the varnish cache.

Updating NULL values to either 'a:0:{}' (map field) or 'N;' (almost any other) on problematic column(s) once also resolves problem.
No patch needed. You can create update hook for this.

This works because problem is appearing only when you are adding field to entities with existing entries.

Part of IS appear to be missing. Thanks

Per @krystalcode's comment in #54, I introduced a utility method to replace the current usages of unserialize() in SqlContentEntityStorage

+  /**
+   * Safely unserializes data.
+   *
+   * @param string|null $value
+   *   The serialized string.
+   *
+   * @return mixed|null
+   *   The unserialized data, or null if the data is invalid.
+   */
+  public static function safeUnserialize($value) {
+    // Return null if the value is an empty string or null.
+    if ($value === '' || $value === null) {
+      return null;
+    }
+
+    // Attempt to unserialize the value.
+    $result = @unserialize($value);
+
+    // Check if unserialize resulted in false, indicating failure.
+    // Also ensure the input value wasn't the serialized form of false itself.
+    if ($result === false && $value !== serialize(false)) {
+      return null;
+    }
+
+    // Return the unserialized result.
+    return $result;
+  }
+

NW for adding tests.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -470,7 +470,7 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
-              $values[$id][$field_name][LanguageInterface::LANGCODE_DEFAULT][$property_name] = !empty($definition_columns[$property_name]['serialize']) ? unserialize($record->{$column_name}) : $record->{$column_name};
+              $values[$id][$field_name][LanguageInterface::LANGCODE_DEFAULT][$property_name] = !empty($definition_columns[$property_name]['serialize']) ? SqlContentEntityStorage::safeUnserialize($record->{$column_name}) : $record->{$column_name};

@@ -481,7 +481,7 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
-            $values[$id][$field_name][LanguageInterface::LANGCODE_DEFAULT] = !empty($column['serialize']) ? unserialize($record->{$column_name}) : $record->{$column_name};
+            $values[$id][$field_name][LanguageInterface::LANGCODE_DEFAULT] = !empty($column['serialize']) ? SqlContentEntityStorage::safeUnserialize($record->{$column_name}) : $record->{$column_name};

@@ -595,12 +595,12 @@ protected function loadFromSharedTables(array &$values, array &$translations, $l
-            $values[$id][$field_name][$langcode] = (!empty($column_attributes['serialize'])) ? unserialize($row[$column_name]) : $row[$column_name];
+            $values[$id][$field_name][$langcode] = (!empty($column_attributes['serialize'])) ? SqlContentEntityStorage::safeUnserialize($row[$column_name]) : $row[$column_name];
...
-              $values[$id][$field_name][$langcode][$property_name] = (!empty($column_attributes['serialize'])) ? unserialize($row[$column_name]) : $row[$column_name];
+              $values[$id][$field_name][$langcode][$property_name] = (!empty($column_attributes['serialize'])) ? SqlContentEntityStorage::safeUnserialize($row[$column_name]) : $row[$column_name];

@@ -1261,7 +1261,7 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
-              $item[$column] = (!empty($attributes['serialize'])) ? unserialize($row->$column_name) : $row->$column_name;
+              $item[$column] = (!empty($attributes['serialize'])) ? SqlContentEntityStorage::safeUnserialize($row->$column_name) : $row->$column_name;

With tests added

Thanks for working but would need to be an MR to test

Also was tagged issue summary update which still appears to be needed.

Thanks for a clear issue summary!

Can we get a change record for the new public function that could be used for others. Sure a contrib module out there maybe could leverage it.

The changes in MR 8092 worked great testing them now on a D10.3.6 install. Thanks.

Patch 63 works well on 10.4.0

Added a first draft for a CR.

CR reads well.

Left some small comments on the MR. But super close and will keep an eye out for this one.

Works good on Drupal Version 10.4.1

Appears to be 1 open thread, @berdir should this be postponed on #3421234: Json and PHP serializers should throw exception on failure which is also up for review?

The Link field also has this problem with its serialized options column that can be NULL. See #2871217: Handle NULL URL options in LinkFormatter::buildUrl().

I'm not done working on this, but I need to save my progress because I've been making these changes on my work laptop and I'm about to head home for the weekend.

I agree with the things @berdir said.

this probably shouldn't be public inside the storage, it's not a meant to be a public api?

Yeah, I don't think we should put utility methods in entity storage classes. That's the kind of thing services are for. And we already have a service for serialization. Let's use it.

I'm not sure if we should silently drop invalid values

No, we shouldn't. We should be doing whatever PhpSerialize does. The utility method picked up a few extra things that it did that are outside the scope of this issue. Dropping the invalid values were included. It also included converting empty strings to NULL. That's a behavior change that could impact downstream code and I don't even know what prompted it. I got rid of all of it. Now the safeUnserialize() function only checks the NULL case, then passes the work off to PhpSerialize.

testSafeUnserialize has to be updated because I made that static utility function protected. Also, InitializeSerializedPropTest passes with or without the bug fix. So something about that test is wrong. Edit: I take that back. It does trigger the deprecation warnings when run without the fix. I must have not reset the fix properly the other day.

Also, I don't think we need to postpone this on #3421234. We just need to use PhpSerialize. Then it can do whatever it's going to do and the entity storage will benefit from those improvements.

Implementing PhpSerialize and injecting it into the class resulted in hundreds of deprecation errors. I didn't want to deal with that, so I dropped the idea. So now the only thing this change does is check for the NULL case, which is what it was originally about. If someone wants to pick up possible improvements to unserialization that might happen in PhpSerialize in the future, then they can do that in a separate issue. Let's keep this about fixing the NULL errors, which is a problem specific to how Drupal interacts with the database.

I deleted the tests that were written previously. One was no longer relevant because of the change away from a static function. I felt that the other one wasn't focused enough. It tried to replicate the older steps to reproduce the problem which involved making an entity, then later adding a serialized base field to the entity type. Basically, it was focused on testing the symptom, not the actual problem. I wrote a new test that checks nullable fields in both dedicated and shared tables.

I don't know if the CR is necessary anymore. It was written because a new static helper function was being added, but that's no longer true.

Left a small comment if it can be expanded. Agree though this may not need a CR.

The feedback was addressed. Thank you to @smustgrave for prioritizing the review of this issue.

Thanks! Feedback appears to be addressed

Ran the test-only job

1 test triggered 2 PHP deprecations:
1) /builds/issue/drupal-3300404/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:485
unserialize(): Passing null to parameter #1 ($data) of type string is deprecated
Triggered by:
* Drupal\KernelTests\Core\Entity\FieldSqlStorageTest::testNullSerializedFieldLoad
  /builds/issue/drupal-3300404/core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php:585
2) /builds/issue/drupal-3300404/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1264
unserialize(): Passing null to parameter #1 ($data) of type string is deprecated
Triggered by:
* Drupal\KernelTests\Core\Entity\FieldSqlStorageTest::testNullSerializedFieldLoad
  /builds/issue/drupal-3300404/core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php:585
OK, but there were issues!
Tests: 9, Assertions: 142, Deprecations: 2.
Exiting with EXIT_CODE=0

Haven't deleted the CR just in case but rest LGTM.

Added an MR comment. Sorry I should have spotted this earlier but the method name is problematic as it does nothing to address security issues with unserialize even though it sounds like it does.

Feedback has been addressed. Setting to Needs Review for a 3rd opinion on the change.

I've updated the issue summary and CR with the new name.

I've updated the contribution record for this issue.

I've updated the issue summary and CR with the new name.

Thank you. Sorry I forgot about doing that.

Feedback for a new name appears to be addressed Thanks!

Committed 4a32768 and pushed to 11.x. Thanks!

I think the change record needs to clarify that this change is specific to \Drupal\Core\Entity\Sql\SqlContentEntityStorage. As it reads to me it implies that all instances of unserialize in core are affected.

I updated the CR. I believe that I made it clear that this change only affects SqlContentEntityStorage.

@dcam, thanks for that. I did more work on restructuring the Change Record so that the change is the first thing mentioned and there is a before and after section. None of that should have changed the meaning. If I made a mistake, just edit it as needed.

I published the change record.