Replace SearchBlock service properties with service closures

Put up a draft MR to see if tests pass, and they do.

Tagging for Needs CR for the constructor argument deprecations, will add that later. I know there was discussion in the original issue about typing, so we can bring that over here. For now, I added a return type declaration to the closures.

One thought that doesn't apply to SearchBlock, but if we continue to use closures for services like this in plugins (and similar), will probably need to be careful with any classes that can be serialized and make sure to override __sleep() and __wakeup().

> One thought that doesn't apply to SearchBlock

Good thought and I'm not even sure it doesn't apply to that. There's a reason that \Drupal\Core\DependencyInjection\DependencySerializationTrait is used all over the place. I think it would be fairly easy to extend that trait and support closures? It's just an extra elseif to check if something is a closure and it's return value is a service? do we need reflection to only call closures without arguments?

Good thought and I'm not even sure it doesn't apply to that. There's a reason that \Drupal\Core\DependencyInjection\DependencySerializationTrait is used all over the place. I think it would be fairly easy to extend that trait and support closures?

I missed that PluginBase does use DependencySerializationTrait, so SearchBlock could be serialized.

As for serialization/unserialization, I think you're right. It could look something like this (untested):

  protected $_serviceClosureIds = [];

  public function __sleep(): array {
    $vars = get_object_vars($this);
    try {
      $container = \Drupal::getContainer();
      $reverse_container = $container->get(ReverseContainer::class);
      foreach ($vars as $key => $value) {
        if (!is_object($value) || $value instanceof TranslatableMarkup) {
          // Ignore properties that cannot be services.
          continue;
        }
        if ($value instanceof EntityStorageInterface) {
          // If a class member is an entity storage, only store the entity type
          // ID the storage is for, so it can be used to get a fresh object on
          // unserialization. By doing this we prevent possible memory leaks
          // when the storage is serialized and it contains a static cache of
          // entity objects. Additionally we ensure that we'll not have multiple
          // storage objects for the same entity type and therefore prevent
          // returning different references for the same entity.
          $this->_entityStorages[$key] = $value->getEntityTypeId();
          unset($vars[$key]);
        }
        elseif ($value instanceof \Closure && ($service_id = $reverse_container->getId(($value)()))) {
          $this->_serviceClosureIds[$key] = $service_id;
          unset($vars[$key]);
        }
        elseif ($service_id = $reverse_container->getId($value)) {
          // If a class member was instantiated by the dependency injection
          // container, only store its ID so it can be used to get a fresh
          // object on unserialization.
          $this->_serviceIds[$key] = $service_id;
          unset($vars[$key]);
        }
      }
    }
    catch (ContainerNotInitializedException) {
      // No container, no problem.
    }

    return array_keys($vars);
  }

  public function __wakeup(): void {
    // Avoid trying to wakeup if there's nothing to do.
    if (empty($this->_serviceIds) && empty($this->_entityStorages)) {
      return;
    }
    $container = \Drupal::getContainer();
    foreach ($this->_serviceIds as $key => $service_id) {
      $this->$key = $container->get($service_id);
    }
    $this->_serviceIds = [];

    foreach ($this->_serviceClosureIds as $key => $service_closure_id) {
      $this->$key = static fn() => $container->get($service_closure_id);
    }

    if ($this->_entityStorages) {
      /** @var \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager */
      $entity_type_manager = $container->get('entity_type.manager');
      foreach ($this->_entityStorages as $key => $entity_type_id) {
        $this->$key = $entity_type_manager->getStorage($entity_type_id);
      }
    }
    $this->_entityStorages = [];
  }

Should that be done here? Or another issue?

Lets do another issue? We'll likely need to add a separate test for it anyway.

do we need reflection to only call closures without arguments?

Interesting. It seems possible that a class could have a property whose value is a closure that takes at least 1 argument, so trying to call that closure without arguments during serialization would cause an error. I don't see a way to check for that other than using relfection on the closure.

Issue to handle serialization: 📌 Handle serialization and unserialization of service closures Active