Cannot quickedit an image or date field [#2475483]

Problem/Motivation

Steps to reproduce:
Create a node, include an image.
Attempt to quickedit the image field.

You'll get "Could not load the form for Image, either due to a website problem or a network connection problem.
Please try again"
from the JS.

To see the real error, hit quickedit/form/node/1/field_image/en/full directly.

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 1300 of core/lib/Drupal/Core/Database/Connection.php).
serialize(Array)
Drupal\Component\Serialization\PhpSerialize::encode(Array)
Drupal\Core\KeyValueStore\DatabaseStorageExpirable->setWithExpire('form-c2H3CRTm__sK0neXm34p0yJzALxIIFfIC1gbaEnaHsI', Array, 21600)
Drupal\Core\Form\FormCache->setCache('form-c2H3CRTm__sK0neXm34p0yJzALxIIFfIC1gbaEnaHsI', Array, Object)
Drupal\Core\Form\FormBuilder->setCache('form-c2H3CRTm__sK0neXm34p0yJzALxIIFfIC1gbaEnaHsI', Array, Object)
Drupal\Core\Form\FormBuilder->processForm('quickedit_field_form', Array, Object)
Drupal\Core\Form\FormBuilder->buildForm('Drupal\quickedit\Form\QuickEditFieldForm', Object)
Drupal\quickedit\QuickEditController->fieldForm(Object, 'field_image', 'en', 'full', Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->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)

This does not happen for the tags, (field_tags), but does also happen if you add a date field, so it's not just images.

Proposed resolution

Remaining tasks

User interface changes

API changes

Comment	File	Size	Author
#23	2475483.23.patch	3.98 KB	alexpott
#23
#23	19-23-interdiff.txt	3.62 KB	alexpott
#19	validator-2475483-19.patch	5.44 KB	Arla
#19
#19	validator-2475483-19-TEST_ONLY.patch	2.59 KB	Arla
#19
#18	validator-2475483-18.interdiff.txt	1.02 KB	Arla
#18	validator-2475483-18.patch	2.85 KB	Arla
#18
#15	validator-2475483-15.patch	2.98 KB	jeroen.b
#15
#1	validator-2475483-1.patch	2.79 KB	Berdir
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 21 April 2015 at 20:48

Status:

Active

» Needs review

File	Size
validator-2475483-1.patch	2.79 KB

Found it.

Validator is not a service, so the trait has no power over it.

Comment #2

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 21 April 2015 at 20:54

Built my own serializer to find it ;)

https://gist.github.com/Berdir/7ba4cf0f6c4bcbd9a4a3

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 22 April 2015 at 06:29

So this was a regression introduced by #2429037: Allow adding entity level constraints.

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 22 April 2015 at 06:30

We already have test coverage for in-place editing fields. Just not for image or date fields. What is special about them, i.e. why does this only happen for those field types?

Comment #5

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 22 April 2015 at 07:03

Not completely sure, different kind of validations maybe, it should really break for everything, the validator is always injected.

Comment #6

yched CreditAttribution: yched commented 22 April 2015 at 08:57

Wondering if we should put the gist from #2 in core somehow. When those errors happen, they are hell to debug :-(

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 22 April 2015 at 09:11

#6++

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 22 April 2015 at 09:11

#5: I wonder if it's just that image/date use the form cache, therefore cause form serialization, and the other fields' widgets don't end up in the form cache, hence don't have this problem?

Comment #9

miro_dietiker

Switzerland

CreditAttribution: miro_dietiker at MD Systems GmbH commented 23 April 2015 at 13:15

Ran into this issue with a multi value wysiwyg enabled long text field. Solved it.

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 23 April 2015 at 13:51

@miro_dietiker: any chance that multi-value WYSIWYG-enabled long text field was using the form cache/form state in some way?

Comment #11

effulgentsia CreditAttribution: effulgentsia at Acquia commented 23 April 2015 at 16:55

Re #10: very likely. If by multivalued, it was "unlimited", then the "Add another item" button adds #ajax to the form, which brings in form caching / state persistence. So possibly this bug applies to any "unlimited"-valued field type.

Comment #12

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 23 April 2015 at 17:59

Yes, and images/files have the #ajax-ified upload button
What about date? Maybe the datepicker? But that's just jQuery...

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 23 April 2015 at 18:18

#12: any chance it was \Drupal\config_translation\FormElement\DateFormat? That uses #ajax.

Comment #14

miro_dietiker

Switzerland

CreditAttribution: miro_dietiker at MD Systems GmbH commented 23 April 2015 at 19:49

Yeah, it was an unlimited long text field with wysiwyg, sry. :-)
And it was just regularly attached to the content type article.

Comment #15

jeroen.b CreditAttribution: jeroen.b at .VDMi/ commented 14 May 2015 at 11:41

File	Size
validator-2475483-15.patch	2.98 KB

Reroll

Comment #16

14 May 2015 at 12:11

Status:

Needs review

» Needs work

The last submitted patch, 15: validator-2475483-15.patch, failed testing.

Comment #17

jeroen.b CreditAttribution: jeroen.b at .VDMi/ commented 14 May 2015 at 13:33

Seems like the patch isn't the solution anymore.
I still get the original error on HEAD though, and this patch fixes that error for me.

Comment #18

Arla

they/them

CreditAttribution: Arla at MD Systems GmbH commented 19 May 2015 at 07:13

Status:	Needs work	» Needs review
Issue tags:		+Needs tests

File	Size
validator-2475483-18.patch	2.85 KB

validator-2475483-18.interdiff.txt	1.02 KB

Looks like last reroll had some bad merge resolutions.

Comment #19

Arla

they/them

CreditAttribution: Arla at MD Systems GmbH commented 19 May 2015 at 08:08

Issue tags:

-Needs tests

File	Size
validator-2475483-19-TEST_ONLY.patch	2.59 KB

validator-2475483-19.patch	5.44 KB

Added test.

Comment #20

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 19 May 2015 at 08:22

Priority:

Normal

» Major

Thank you very much for the test coverage! Waiting for testbot verdict.

Patch looks good.

Comment #21

19 May 2015 at 08:37

The last submitted patch, 19: validator-2475483-19-TEST_ONLY.patch, failed testing.

Comment #22

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 19 May 2015 at 08:51

Status:

Needs review

» Reviewed & tested by the community

Comment #23

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 21 May 2015 at 13:15

Status:

Reviewed & tested by the community

» Needs review

File	Size
19-23-interdiff.txt	3.62 KB
2475483.23.patch	3.98 KB

The problem appears to be coming from serializing things which are container aware or have the class resolver injected. Here is a more robust solution since we can not force everyone to inject the right thing and have tests to find these edge cases.

Comment #24

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 21 May 2015 at 13:21

Assigned:

Unassigned

» Berdir

Wow, ok, thanks.

Assigning to @Berdir for review.

Comment #25

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 21 May 2015 at 16:06

Assigned:	Berdir	» Unassigned
Status:	Needs review	» Reviewed & tested by the community

That's fine with me as well, although we probably still serialize quite a bit more than we did with the other patch?

Comment #26

yched CreditAttribution: yched commented 21 May 2015 at 17:26

That means we have a Validator that holds a reference to the db connection ? Isn't that weird ?

Comment #27

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 21 May 2015 at 18:26

@yched I don't think we have that - I checked all the validators with injection - I think db serialize is coming from trying to serialize the container.

Comment #28

yched CreditAttribution: yched commented 21 May 2015 at 20:42

I think db serialize is coming from trying to serialize the container.

But given Drupal\Core\DependencyInjection\Container::__sleep(), serializing our container should throw an exception upfront without going deeper ?

Comment #29

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 21 May 2015 at 22:20

The test failures of https://qa.drupal.org/pifr/test/1050593 are pretty clear if you click in.
One request, it's the container. The other is Settings.

Comment #30

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 21 May 2015 at 22:34

It's the same request, actually but yes.

I tried to be nice in __sleep() and just trigger a php warning, but then it gets to the settings object which is part of the container and that results in an exception (which in the test is reported at the end, but it all happens at the same time).

Comment #31

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 21 May 2015 at 22:35

The database serialization occurs because when we serialize the RecursiveValidator returned by TypedDataManager::getValidator we end up serializing the TypedDataManager which as a plugin manager has a cache backend injected which has the database connection injected.

The container serialization occurs because the RecursiveValidator also has a ConstraintValidatorFactory which has the class resolver injected - serializing that causes a container serialization.

Therefore added DependencySerialization to ClassResolver and TypeDataManager fixes the problem.

Comment #32

yched CreditAttribution: yched commented 22 May 2015 at 08:56

Thanks for the explanations, makes sense (sadly :-/)
Also, right I overlooked that Container::__sleep() does spit an error but does not stop execution

RTBC +1, anyway

Comment #33

22 May 2015 at 14:29

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 23: 2475483.23.patch, failed testing.

Comment #34

23 May 2015 at 05:25

Status:

Needs work

» Needs review

tim.plunkett queued 23: 2475483.23.patch for re-testing.

Comment #35

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 23 May 2015 at 05:25

Status:

Needs review

» Reviewed & tested by the community

Failed to clear checkout directory

Comment #36

23 May 2015 at 22:49

miro_dietiker queued 23: 2475483.23.patch for re-testing.

Comment #37

miro_dietiker

Switzerland

CreditAttribution: miro_dietiker at MD Systems GmbH commented 23 May 2015 at 22:49

Still applies here.

Comment #38

catch

he/him

English

CreditAttribution: catch commented 26 May 2015 at 09:39

Status:

Reviewed & tested by the community

» Fixed

Committed/pushed to 8.0.x, thanks!

Comment #39

26 May 2015 at 09:40

catch committed 0c80f49 on 8.0.x

Issue #2475483 by Arla, alexpott, Berdir, jeroen.b: Cannot quickedit an...

Comment #40

9 June 2015 at 09:44

Status:

Fixed

» Closed (fixed)

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

Comment #41

SiliconMind CreditAttribution: SiliconMind commented 14 December 2015 at 20:10

Version:	8.0.x-dev	» 8.0.1
Status:	Closed (fixed)	» Active

Still present in 8.0.1 for image fields.
Unable to quick edit image field. Clicking "Delete" button in quick edit popup for image field produces LogicException:

Uncaught PHP Exception 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." at /var/www/stuxnet/core/lib/Drupal/Core/Database/Connection.php line 1433

Comment #42

Arla

they/them

CreditAttribution: Arla at MD Systems GmbH commented 14 December 2015 at 22:05

Status:

Active

» Closed (fixed)

Yes, confirmed #41. But that's about the Remove button, so it's a different problem. Could you open a new issue for this, SiliconMind? In any case, please do not reopen closed issues.

Comment #43

SiliconMind CreditAttribution: SiliconMind commented 14 December 2015 at 23:08

@Arla, I'd argue on that. It's still the same problem because in order to upload new image you first need to remove the existing image. And the exception thrown is the same. So why create new issue for the same bug? The point is that you can not edit the image field and this is what title and issue description says. The problem was not fixed with the patch and what's more, it was the bot that closed the issue which apparently was not tested fully when patch was applied.