Problem/Motivation

Getting

LogicException: Settings can not be serialized. This probably means you are serializing an object that has an indirect reference to the Settings object. Adjust your code so that is not necessary. in Drupal\Core\Site\Settings->__sleep() (line 67 of core/lib/Drupal/Core/Site/Settings.php).

When try to render a form with datetime element.

Steps to reproduce

1. Create a form with an element below:


  $form['sample'] = array(
        '#title' => 'Sample field,
        '#type' => 'datelist',
        '#default_value' => new DrupalDateTime('2000-01-01 00:00:00'),
        '#date_part_order' => array('month', 'day', 'year', 'hour', 'minute', 'ampm'),
        '#date_text_parts' => array('year'),
        '#date_year_range' => '2010:2020',
        '#date_increment' => 15,
      );

2. Try to visit the form UL.

Full exception trace if it helps:

LogicException: Settings can not be serialized. This probably means you are serializing an object that has an indirect reference to the Settings object. Adjust your code so that is not necessary. in Drupal\Core\Site\Settings->__sleep() (line 67 of core/lib/Drupal/Core/Site/Settings.php).
serialize(Array)
Drupal\Component\Serialization\PhpSerialize::encode(Array)
Drupal\Core\KeyValueStore\DatabaseStorageExpirable->setWithExpire('form-zRDOGhal0mExQf8tb4Wt4jN283dcztARg82WW3doFpc', Array, 21600)
Drupal\Core\Form\FormCache->setCache('form-zRDOGhal0mExQf8tb4Wt4jN283dcztARg82WW3doFpc', Array, Object)
Drupal\Core\Form\FormBuilder->setCache('form-zRDOGhal0mExQf8tb4Wt4jN283dcztARg82WW3doFpc', Array, Object)
Drupal\Core\Form\FormBuilder->processForm('error_style_form', Array, Object)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Symfony_Component_HttpKernel_HttpKernel_Proxy->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_StackMiddleware_Session_Proxy->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_StackMiddleware_KernelPreHandle_Proxy->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

Proposed resolution

Looks like #2199795: Make the Settings class prevent serialization of actual settings has introduced the exception and might need some test coverage to prove it.

Looks like the flow is DrupalDateTime => TranslationManager => translator => CustomStrings => Settings. So if we might need to remove the Settings as a dependency for CustomStrings.

Remaining tasks

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Priority: Normal » Major
Status: Active » Needs review
Related issues: +#2443351: Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters.
FileSize
1.45 KB

#2443351: Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters. has the fix for all these type of issues, for this issue, just removing Settings from CustomStrings solves the problem.

dawehner’s picture

Looks great! Well, I've seen that kind of change before :)

I guess in order to ensure that this doesn't happen anymore we need some form of test? I guess a form with simply the CustomString service injected should be enough ...

vijaycs85’s picture

Issue tags: +Needs tests

#4: Yep, I'm working on it.

JayeshSolanki queued 3: 2502195-3.patch for re-testing.

amateescu’s picture

larowlan’s picture

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

FileSize
4.46 KB
3.01 KB

Thanks @amateescu and @larowlan. Here is the test that's failing because of #2502923: PHP warning on Drupal\Core\Datetime\Element\Datelist::incrementRound()

Status: Needs review » Needs work

The last submitted patch, 10: 2502195-10.patch, failed testing.

tim.plunkett’s picture

Title: Regression: Form throws LogicException when trying to render a form with object as an element's default value. » \Drupal\Core\StringTranslation\Translator\CustomStrings should be serializable, but contain \Drupal\Core\Site\Settings which is not
Component: forms system » base system

What issue queue component is StringTranslation?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Reuploading the previous test-only patch to see what it does now.

@effulgentsia, @catch, @Cottser, @alexpott and I agreed this is definitely a major bug if it is still reproducible. Not being able to have a date list in a form is a significant developer-facing bug. Can we also test the date list widget in the UI to see if it's reproducible there?

Status: Needs review » Needs work

The last submitted patch, 14: 2502195-12-FAIL.patch, failed testing.

xjm’s picture

Issue tags: +D8 major triage deferred

For #14.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.05 KB
643 bytes

Updated patch. Should throw exception now.

Status: Needs review » Needs work

The last submitted patch, 18: 2502195-18.patch, failed testing.

vijaycs85’s picture

Issue tags: +D8MI, +sprint

I believe, this can be added to D8MI sprint.

larowlan’s picture

Patch at #18 is test only, patch with fix at #3 causes ( ! ) Fatal error: Cannot use object of type Drupal\Core\Site\Settings as array in /vagrant/app/core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.php on line 34

larowlan’s picture

Ignore #21, fixed after a cache rebuild.

So we just need a patch combining #18 and #3

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.5 KB
dawehner’s picture

+++ b/core/modules/datetime/src/Tests/DateTimeFormInjectionTest.php
@@ -0,0 +1,119 @@
+
+namespace Drupal\datetime\Tests;
...
+class DateTimeFormInjectionTest extends KernelTestBase implements FormInterface {

This is the old kernel test base, it would be nice to not introduce a new one of those.

larowlan’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
132.04 KB

nodding

alexpott’s picture

Any reason why we're not using DependencySerializationTrait here? In works a treat btw.

larowlan’s picture

Assigned: Unassigned » larowlan

sure, doing the tests so will do that at same time

larowlan’s picture

switching to use the trait

The last submitted patch, 28: 2502195-custom-strings-settings-2.test-only.patch, failed testing.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @larowlan!!!

Looks very simple as the changes needed are in trait which has been used in other places. Both concerns on #24 and #26 have been address. This is good to go!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 1e2f37a to 8.3.x and 889f09f to 8.2.x. Thanks!

  • alexpott committed 1e2f37a on 8.3.x
    Issue #2502195 by vijaycs85, larowlan: \Drupal\Core\StringTranslation\...

  • alexpott committed 889f09f on 8.2.x
    Issue #2502195 by vijaycs85, larowlan: \Drupal\Core\StringTranslation\...

Status: Fixed » Closed (fixed)

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