Make serializer customizable for Cache\DatabaseBackend

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I'm also still not convinced (see #839444-111: Make serializer customizable for Cache\DatabaseBackend from 2017) about adding a dependency on the Serialization component in the cache system, since the serialization component has a dependency on Symfony YAML.

This is still the case, but I had another look and I don't think it should hold back this patch. It makes sense to use the serialization component here. If we don't want the dependency on Symfony YAML, we should consider splitting the YAML serialization into a separate SerializationYamlComponent, or making the YAML dependency suggested instead of required - but that wouldn't change anything here.

So to me, #198 looks good.

The last submitted patch, 203: 839444_10.0.x.patch, failed testing. View results →

The last submitted patch, 205: 839444_10.x.patch, failed testing. View results →

Updated patch for Drupal 10.1.5

"No space left on device"

Drupal CI is down.

Yes, we have GitlabCI now. Rerolled #209 in the MR.

one test fails Drupal\KernelTests\Core\DependencyInjection\AutowireTest

I left a couple of comments.

Thanks for fixing the tests!

@catch already agreed on the patch in #201 and we use it for years on multiple production sites. So, I "carefully" set it to RTBC.

Not going to weigh in on whether this can be approved as @catch has already done so, but setting back to NW for a small nitpick. Feel free to RTBC again once that is fixed.

fixed version string and converted properties to constructor promotion

From a code perspective this looks good. Conceptually, I haven't been involved in this discussion at all so I'm still rolling with the approval given in #201

There is still a problem with argument ordering. The required method argument is placed after the optional. This is against the coding standard because this leads to optional argument becoming required.

@voleger which argument? All added are optional, and ladt commit fixed it

I don't see it either, to me it shows as the last argument and is marked as optional. If #222 was a mistake than feel free to set back to RTBC

The deprecation message says that $serializer becomes a required field in drupal:11.0.0. The problem is that the previous argument $max_rows has to remain optional in drupal:11.0.0. We need to keep $max_rows optional.
See https://git.drupalcode.org/project/drupal/-/merge_requests/6296/diffs#51...

The same thing is related to another constructor.
See https://git.drupalcode.org/project/drupal/-/merge_requests/6296/diffs#51...
$settings have to be optional in drupal:11.0.0.

More about required arguments after optional https://php.watch/versions/8.0/deprecate-required-param-after-optional

Thank you! Good point, it requires to shif argument somehow

Both arguments added in #2526150: Database cache bins allow unlimited growth: cache DB tables of gigabytes! →
So very probably it was just happened before out deprecation policy formed (core 8.4) https://www.drupal.org/node/2891281 →

So I reordered $max_rows and deprecated to create without settings

Thanks, the changes are reasonable. Good point on pre-deprecation policy changes - agree that settings need to be required.
+1 for RTBC

Nice to get this done after (checks notes) 14 years!

Let's keep going in #3014514: Make igbinary the default serializer if available, it saves 50% time on unserialize and memory footprint → .

This causes warnings when doing a site-install using Drush...

PHP Fatal error:  Uncaught Error: Call to a member function get() on null in /Volumes/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php:1410
Stack trace:
#0 /Volumes/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php(688): Drupal\Core\DrupalKernel->getHttpKernel()
#1 /Volumes/dev/sites/drupal8alt.dev/vendor/drush/drush/src/Boot/DrupalBoot8.php(326): Drupal\Core\DrupalKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#2 [internal function]: Drush\Boot\DrupalBoot8->terminate()
#3 {main}
  thrown in /Volumes/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php on line 1410

Fatal error: Uncaught Error: Call to a member function get() on null in /Volumes/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php:1410
Stack trace:
#0 /Volumes/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php(688): Drupal\Core\DrupalKernel->getHttpKernel()
#1 /Volumes/dev/sites/drupal8alt.dev/vendor/drush/drush/src/Boot/DrupalBoot8.php(326): Drupal\Core\DrupalKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#2 [internal function]: Drush\Boot\DrupalBoot8->terminate()
#3 {main}
  thrown in /Volumes/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php on line 1410

It's because Drush has a DrupalKernel without a container so we should check for that too.

Opened 🐛 Drush error on site-install Active to fix this.

This issue conflicts heavily with 📌 Replace REQUEST_TIME in services Fixed - one thing we did there was handle string typed $max_rows in DatabaseBackend which this issue didn't handle. That could lead to a BC break.

See https://git.drupalcode.org/project/drupal/-/merge_requests/4302#note_259560

Let's either open a quick follow-up for string-typed max rows or roll this one back. Just re-opening for now.

One other review point from 📌 Replace REQUEST_TIME in services Fixed that's also relevant here is we should add the serialization component classes that are loaded on a regular bootstrap (or especially page cache hit) to the core/composer.json classmap so they're optimized in the autoloader.

Opened 🐛 DatabaseBackend doesn't handle string typed $max_rows with $serializer Active I'll get an MR going.

Opened 📌 Add relevant serializer classes to the classmap Active too.

And a novice follow up for constructor prop promotion 📌 Use constructor property promotion on $serializer in Drupal\Core\Cache\DatabaseBackend Active

Follow-ups are filed, btw $max_rows could be fixed in 📌 Replace REQUEST_TIME in services Fixed

CRs for this issue are also still in draft (maybe that's right?) but do need version updates.

It also may be wise to combine the CRs around the new dependencies once 3113971 is in?

Thanks to @acbramley who confirmed the three change records here are correct.

They are now published with correct dates.

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