DrupalDateTime serialization issue [#3187004]

Problem/Motivation

When serializing DrupalDateTime object the following exception is thrown:

LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in Drupal\Core\Database\Connection->__sleep() (line 1574 of web/core/lib/Drupal/Core/Database/Connection.php).

This is caused by formatTranslationCache property of the object. If there is another language selected than English, this property will contain references to stringTranslation service that it its turn can have database service references if you use locale module.

Steps to reproduce

PHP version: 7.3.24
Web Server: Apache
OS: Centos 7

1. Install Drupal 8.9.x
1a. Add language other than "English".
1b. Make sure that there are translations of months, days of the weeks in the database.
2. Install Commerce module 2.21
3. Install Commerce Shipping module.
4. Use delivery_date property of ShippingRate class. I am not aware of any contrib module that does it, but my custom shipping method provides rates that depend on date. The instance of this class is added to ShippingRates widget that is displayed on checkout form.
5. Install Commerce Promotion or Commerce GiftCard module, so that it is possible to run ajax callbacks on checkout for.
6. Get to the step where you can select the shipping rate and use the language other than English.
7. Try to add coupon/card code.
8. Observe the exception described in the "Problem Motivation".
9. Change to English language, try to do the same, everything should work as expected.

I guess there are easier steps to reproduce, like creating a form with ajax callback that has DrupalDateTime object for any form element.

Proposed resolution

Implement __sleep() method in DrupalDateTime class like:

  public function __sleep() {
    return ['langcode', 'dateTimeObject'];
  }

Remaining tasks

Write tests, make sure that no other properties of the class are needed in serialized version of the instance.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Not needed.

Comment	File	Size	Author
#31	3187004-11.x-MR-6334--20240125-1.diff	2.27 KB	recrit
#19	interdiff_13-19.txt	522 bytes	ranjith_kumar_k_u
#19	3187004-19.patch	2.19 KB	ranjith_kumar_k_u
#11	3187004-drupal-date-time-serialization-11.patch	2.2 KB	a.dmitriiev

Issue fork drupal-3187004

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

4 hidden branches

3187004-11.x

changes, plain diff MR !6334

Check out this branch for the first time

Check out existing branch, if you already have it locally

11.x

compare

Check out this branch for the first time

Check out existing branch, if you already have it locally

3187004-drupaldatetime-serialization-issue

changes, plain diff MR !112

Check out this branch for the first time

Check out existing branch, if you already have it locally

3187004-drupal9-rebased

changes, plain diff MR !1340

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

7 December 2020 at 16:36

a.dmitriiev created an issue. See original summary.

Comment #2

7 December 2020 at 16:43

a.dmitriiev opened merge request !112

Comment #3

a.dmitriiev commented 7 December 2020 at 16:45

Issue summary:

View changes

Comment #4

larowlan

🇦🇺🏝.au GMT+10

commented 7 December 2020 at 19:26

Status:	Active	» Needs work
Issue tags:		+Needs tests, +Bug Smash Initiative

Thanks, patch looks straightforward, just need a test

Comment #5

25 February 2021 at 16:00

mohit_aghera made their first commit to this issue’s fork.

Comment #6

mohit_aghera commented 26 February 2021 at 04:22

Issue tags:

-Needs tests

Comment #7

mohit_aghera commented 28 February 2021 at 03:54

Status:

Needs work

» Needs review

Comment #8

larowlan

🇦🇺🏝.au GMT+10

commented 2 March 2021 at 09:32

Looking good, would be good to somehow see a failing test patch

Comment #9

matroskeen

Ukrainian

🇺🇦 Ukraine, Lutsk

commented 2 March 2021 at 09:51

Status:

Needs review

» Needs work

I was recently dealing with serializing issues in migration tests, so I have thrown in my 2 cents.
Also, I'm +1 to upload a test-only patch and moving to NW to address remaining items.

Comment #10

matroskeen

Ukrainian

🇺🇦 Ukraine, Lutsk

commented 10 March 2021 at 08:19

For some reason, GitLab didn't send me a notification about your comment @mohit_rocks...
I have just one more question in gitlab ;)

I'm also wondering why the target branch is 8.9.x. Shouldn't it be 9.2.x?

Comment #11

a.dmitriiev commented 25 October 2021 at 16:13

Status	File	Size
new	3187004-drupal-date-time-serialization-11.patch	2.2 KB

Re-rolled patch for 9.2.x

Comment #12

neclimdul

He/Him

commented 25 October 2021 at 19:46

Couldn't figure out what was going on in the merge request. 1000s of changes and commits so maybe it needs to rebased or something.

If I understand the problem correctly, this is just on the property caching localisation. Looking at the patch, could we avoid a lot of complexity with the trait and wakeup stuff and just empty the cache during serialization and assume it can(and should) be rebuilt after serialization? The rest shouldn't really matter.

Comment #13

26 October 2021 at 07:13

a.dmitriiev opened merge request !1340

Comment #14

26 October 2021 at 07:14

a.dmitriiev closed merge request !112

Comment #15

a.dmitriiev commented 26 October 2021 at 07:16

Status:

Needs work

» Needs review

I created another MR to target to 9.3.x branch.

In my particular case the DrupalDateTime object was used in ajax form and the form cache was broken because of date object serialization, because localization has database service.

Comment #16

a.dmitriiev commented 26 October 2021 at 07:17

Version:

8.9.x-dev

» 9.3.x-dev

Comment #17

26 October 2021 at 07:17

Version:

9.3.x-dev

» 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #18

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 13 February 2022 at 23:39

Thanks for the MR and patch. If I understand correctly, the patch is for 8.9 and the MR is for 9.4.

I looked at the patch in #11 and noticed a couple minor nitpicks:

```
+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -21,6 +22,10 @@
+    __wakeup as defaultWakeup;
+    __sleep as defaultSleep;
```
Nitpick: Ordering doesn't match function ordering (__sleep then __wakeup) but I see this is the same as core/modules/forum/src/ForumManager.php so I guess it should stay.

+++ b/core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php
@@ -239,4 +239,23 @@ public function testGetPhpDateTime() {
+   * Test to avoid serialization of formatTranslationCache

Period is missing.

The MR has a gazillion changes so I'm not sure what to do there: https://git.drupalcode.org/project/drupal/-/merge_requests/112.diff

Comment #19

ranjith_kumar_k_u commented 14 February 2022 at 07:05

Status	File	Size
new	3187004-19.patch	2.19 KB
new	interdiff_13-19.txt	522 bytes

Just fixed CS error

Comment #20

a.dmitriiev commented 14 February 2022 at 10:59

Kristen, the MR 122 is already closed because it is outdated. The new MR is here https://git.drupalcode.org/project/drupal/-/merge_requests/1340

Comment #21

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 16 February 2022 at 20:19

Status:

Needs review

» Needs work

Moving back to needs work based on feedback above.

Comment #22

16 February 2022 at 20:19

Version:

9.4.x-dev

» 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #23

16 February 2022 at 20:19

Version:

9.5.x-dev

» 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #24

16 February 2022 at 20:19

Version:

10.1.x-dev

» 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #25

27 January 2024 at 01:14

recrit made their first commit to this issue’s fork.

Comment #26

27 January 2024 at 01:24

recrit changed the visibility of the branch 11.x to hidden.

Comment #27

27 January 2024 at 01:25

recrit opened merge request !6334

Comment #28

27 January 2024 at 01:25

recrit changed the visibility of the branch 3187004-drupal9-rebased to hidden.

Comment #29

recrit commented 27 January 2024 at 01:26

3 files were hidden/shown/deleted

Status	File	Size
hidden	3187004-drupal-date-time-serialization-11.patch	2.2 KB
hidden	3187004-19.patch	2.19 KB
hidden	interdiff_13-19.txt	522 bytes

Comment #30

recrit commented 27 January 2024 at 01:32

Status:

Needs work

» Needs review

I created a new MR 6334 based on 11.x. The following have been address.

- #18 code cleanup - DONE..

- Regarding MR1340 comment "Can we replace this by just settings the property definition to protected $formatTranslationCache = [];" - DONE

- Regarding MR1340 comment "This class really doesn't need any of the service detection magic. Lets just do this and get rid of the trait." - This does need the DependencySerializationTrait for the service "string_translation" stored in protected $stringTranslation.

Comment #31

recrit commented 27 January 2024 at 01:35

Status	File	Size
new	3187004-11.x-MR-6334--20240125-1.diff	2.27 KB

attaching a static patch for composer builds.

Needs review

» Reviewed & tested by the community

All feedback appears to be addressed!

Comment #35

larowlan

🇦🇺🏝.au GMT+10

commented 18 February 2024 at 21:50

Issue credits

Comment #36

18 February 2024 at 21:56

larowlan closed merge request !6334

Comment #37

18 February 2024 at 21:56

larowlan committed 6567f4f0 on 10.2.x

Issue #3187004 by recrit, a.dmitriiev, mohit_aghera, ranjith_kumar_k_u,...

Comment #38

18 February 2024 at 21:56

larowlan committed 6db12006 on 11.x

Issue #3187004 by recrit, a.dmitriiev, mohit_aghera, ranjith_kumar_k_u,...

Comment #39

larowlan

🇦🇺🏝.au GMT+10

commented 18 February 2024 at 21:56

Version:	11.x-dev	» 10.2.x-dev
Status:	Reviewed & tested by the community	» Fixed

Committed to 11.x and backported to 10.2.x - thanks!

Comment #40

3 March 2024 at 21:59

Status:

Fixed

» Closed (fixed)

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