Problem/Motivation

When you have a form array with a render array generated by an embedded view, then it fails to serialize because it is trying to serialize the database connection.

The serialization tree goes through \Drupal\views\Plugin\views\display\Embed and then \Drupal\views\Plugin\views\query\Sql, I think at least the Sql one already also shouldn't be serialized, doesn't seem to make sense, but that's likely much harder to fix than this.

Proposed resolution

Use the dependency serialization trait on all those SqlDate classes, not sure why only the PostgreSQL one fails like that.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 views-sql-date-serialize-3020902-8.patch2.02 KBBerdir
#5 views-sql-date-serialize-3020902-3.patch2.67 KBBerdir
#5 views-sql-date-serialize-3020902-3-test-only.patch706 bytesBerdir
#2 views-sql-date-serialize-3020902-2.patch1.98 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
views-sql-date-serialize-3020902-2.patch1.98 KB

This patches fixes it for me.

Need to check if I can somehow create a test, you can see the failing TMGMT tests: https://www.drupal.org/pift-ci-job/1148805. I also just realized that SQLite also has test fails like that. https://www.drupal.org/pift-ci-job/1148806

To manually reproduce, install tmgmt_demo, go to admin/tmgmt/sources, request a translation of a node and then switch the translator and you'll see the exception on the ajax request.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Is this fix on the right level? Which object is being serialised? If it is a ViewExecutable how come \Drupal\views\ViewExecutable::__sleep() is not protecting us?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Because the array contains the result of $view->preview() and that has a #pre_render callback that points to \Drupal\views\Plugin\views\display\Embed, so we are bypassing ViewExecutable.

And Embed, like all views plugins, have the DependencySerializationTrait, so I'm not quite sure how it finds the Sql plugin then exactly, but I guess it is hidden in some array of plugins. And Sql does implement that trait too, and I think I understand now why that doesn't see it:

  views.date_sql:
    class: Drupal\views\Plugin\views\query\MysqlDateSql
    arguments: ['@database']
    tags:
       - { name: backend_overridable }
  pgsql.views.date_sql:
    class: Drupal\views\Plugin\views\query\PostgresqlDateSql
    arguments: ['@database']
    public: false
  sqlite.views.date_sql:
    class: Drupal\views\Plugin\views\query\SqliteDateSql
    arguments: ['@database']
    public: false

This uses the generic backend-override feature of our container, and I suspect that's not compatible with DependencySerializationTrait, that probably doesn't know what to do with that thing.

So yeah, it isn't pretty and we should catch this earlier, but I'm guess fixing it properly is going to be far more complex than this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
views-sql-date-serialize-3020902-3-test-only.patch706 bytes
views-sql-date-serialize-3020902-3.patch2.67 KB

Here is a failing test, not very pretty but it does the trick. Note that it only fails *after* calling render() on it, I guess the view isn't fully initialized before that. My code doesn't use buildRenderable() but preview(), which already execute the query.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I added a follow up for this: #3021299: Ensure that aliased/used backend overridable are not set to private. I think the fix for now is okay.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

It's not backend_overrideable it's the fact that the pgsql and sqlite implements are private.

See the code in \Drupal\Core\DependencyInjection\Compiler\DependencySerializationTraitPass

      // Only add the property to services that are public (as private services
      // can not be reloaded through Container::get()) and are objects.
      if (!$definition->hasTag('parameter_service') && $definition->isPublic()) {
        $definition->setProperty('_serviceId', $service_id);
      }

The upshot of this is that we need to adjust the scope of #3021299: Ensure that aliased/used backend overridable are not set to private to deal with private services that have public aliases. Also it means that 

+++ b/core/modules/views/src/Plugin/views/query/MysqlDateSql.php
@@ -3,6 +3,7 @@
+use Drupal\Core\DependencyInjection\DependencySerializationTrait;

@@ -14,6 +15,8 @@
+  use DependencySerializationTrait;
+

is not necessary since this is a public service.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
views-sql-date-serialize-3020902-8.patch2.02 KB

Fair enough, removed that part.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Also updated the follow-up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Reviewed & tested by the community

This is a fine work-around for now and we have #3021299: Ensure that aliased/used backend overridable are not set to private in place for the larger issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 60574dd038 to 8.7.x and b6e5032aba to 8.6.x. Thanks!

  • alexpott committed 60574dd on 8.7.x 
    Issue #3020902 by Berdir, alexpott: PostgresqlDateSql fails to serialize

  • alexpott committed b6e5032 on 8.6.x 
    Issue #3020902 by Berdir, alexpott: PostgresqlDateSql fails to serialize...

Status: Fixed » Closed (fixed)

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