While editing a existing server a LogicException is thrown when I try to change between the "Basic Auth" and the "Standard" Solr Connector.

"A fatal error occurred: 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."

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jseffel created an issue. See original summary.

mkalkbrenner’s picture

Priority: Normal » Critical
Issue tags: +Release blocker

I can reproduce the issue. It didn't exist when we introduced connectors. So I assume that something changed in Search API or Core that adds the database connection here. I need to do some debugging.

Berdir’s picture

Usually that's a form or other class so with an injected plugin manager/collection that doesn't use the DependencySerializationTrait

mkalkbrenner’s picture

That's why we committed #2817341: ConfigurablePluginBase should use PluginDependencyTrait instead of DependencyTrait to Search API and why I'm surprised that the error is back again.

mkalkbrenner’s picture

Project: Search API Solr Search » Search API
Component: Code » General code
Status: Active » Needs review
FileSize
1.44 KB

I don't know exactly what changed in core or somewhere else. But I did some debugging. It turned out that there're two problems and at least the first issue is caused by Search API.
The form serialization finally reaches Drupal\Core\Form\FormCache::setCache().
The serialization of the form_state fails because the storage is a property of Drupal\search_api\Form\ServerForm:

class ServerForm extends EntityForm {

  /**
   * The server storage controller.
   *
   * @var \Drupal\Core\Entity\EntityStorageInterface
   */
  protected $storage;

  /**
   * The backend plugin manager.
   *
   * @var \Drupal\search_api\Backend\BackendPluginManager
   */
  protected $backendPluginManager;

  /**
   * Constructs a ServerForm object.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   * @param \Drupal\search_api\Backend\BackendPluginManager $backend_plugin_manager
   *   The backend plugin manager.
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager, BackendPluginManager $backend_plugin_manager) {
    $this->storage = $entity_type_manager->getStorage('search_api_server');
    $this->backendPluginManager = $backend_plugin_manager;
  }
}

The storage isn't a service and therefor not unset before serialization by DependencySerializationTrait.
If you edit a new server, it works and the services within the storage are unset during serialization.
When you edit an existing server, it seems that there's in addition a database connection which remains and leads to the exception.

I had a look at the various examples in core. Some machine_name exists properties are implemented the same way as currently in Search API and should potentially lead to the same issue :-(

But a significant amount of implementations do it differently and don't require the storage directly anymore.

I added a patch to follow this pattern. This fixes the cachability of the form_state!

Unfortunately that patch doesn't resolve the complete issue. Now the serialization of the form array is finally reached. But it fails and i'm still debugging ...

Status: Needs review » Needs work

The last submitted patch, 5: 2882347_static_call_load.patch, failed testing.

mkalkbrenner’s picture

Status: Needs work » Needs review

No space left on device. lol

Status: Needs review » Needs work

The last submitted patch, 5: 2882347_static_call_load.patch, failed testing.

mkalkbrenner’s picture

Status: Needs work » Needs review
FileSize
433 bytes
1.44 KB

Ups, there's a copy and paste error in the patch in #5.

Please note that the Release blocker tag is about Search API Solr Search 8.x-1.0.

borisson_’s picture

It's weird that this is needed again, but the solution looks solid.

However, let's import The server entity class so that it's just Server::load?

mkalkbrenner’s picture

However, let's import The server entity class so that it's just Server::load?

No, you need to provide something that could be executed elsewhere in the form engine.
I think my solution is the correct one, as this is the newer pattern in core as well.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Release blocker +Contributed project blocker

In that case, let's get this in.

swentel’s picture

IndexForm probably needs that too then, even it's not affected (yet) ?

mkalkbrenner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
698 bytes
2.19 KB

In IndexForm it's different because the storage isn't a property of the form. Nevertheless I extended the patch to be consistent.

Status: Needs review » Needs work

The last submitted patch, 14: 2882347-14.patch, failed testing. View results

mkalkbrenner’s picture

Status: Needs work » Needs review

OK, the fails are not related to the patch!

It seems like Search API test now fail in general on 8.4.x.

@borisson_: can you RTBC it again to get it in quickly?

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
Berdir’s picture

Looks good to me as well.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Not totally happy with removing dependency injection, but not unhappy enough to delay the patch if it fixes a release blocker.
So: committed.
Thanks a lot for your work, everyone – especially Markus!

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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