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
Comment | File | Size | Author |
---|---|---|---|
#28 | 2502195-custom-strings-settings-2.28.patch | 3.66 KB | larowlan |
#28 | 2502195-custom-strings-settings-2.test-only.patch | 2.96 KB | larowlan |
#28 | interdiff.txt | 4.62 KB | larowlan |
#25 | nodding.gif | 132.04 KB | larowlan |
#23 | 2502195-custom-strings-settings-2.23.patch | 4.5 KB | larowlan |
Comments
Comment #1
vijaycs85Comment #2
vijaycs85Comment #3
vijaycs85#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.
Comment #4
dawehnerLooks 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 ...
Comment #5
vijaycs85#4: Yep, I'm working on it.
Comment #7
amateescu CreditAttribution: amateescu as a volunteer commentedThe current issue summary says that #2463321: Serializing the database connection is dangerous and error-prone, make it unserializable again introduced this regression, but that's not accurate, that patch simply changed the exception message. It was introduced by #2199795: Make the Settings class prevent serialization of actual settings :)
Comment #8
larowlan@vijaycs85 check #2481731: Cannot inject DBLog instances into forms - The database connection is not serializable for simple way to test for this
Comment #9
vijaycs85Comment #10
vijaycs85Thanks @amateescu and @larowlan. Here is the test that's failing because of #2502923: PHP warning on Drupal\Core\Datetime\Element\Datelist::incrementRound()
Comment #12
tim.plunkettWhat issue queue component is StringTranslation?
Comment #14
xjmReuploading 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?
Comment #16
xjmFor #14.
Comment #18
vijaycs85Updated patch. Should throw exception now.
Comment #20
vijaycs85I believe, this can be added to D8MI sprint.
Comment #21
larowlanPatch 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
Comment #22
larowlanIgnore #21, fixed after a cache rebuild.
So we just need a patch combining #18 and #3
Comment #23
larowlanComment #24
dawehnerThis is the old kernel test base, it would be nice to not introduce a new one of those.
Comment #25
larowlanComment #26
alexpottAny reason why we're not using DependencySerializationTrait here? In works a treat btw.
Comment #27
larowlansure, doing the tests so will do that at same time
Comment #28
larowlanswitching to use the trait
Comment #30
vijaycs85Thanks @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!
Comment #31
alexpottCommitted and pushed 1e2f37a to 8.3.x and 889f09f to 8.2.x. Thanks!