[PP-1] 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().

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?

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: #3544994: Make service closures serializable in classes using DependencySerializationTrait

Checked some other existing deprecation messages for constructor parameters, and some of them link to the issue and not a change record, so I decided just to do that. Removing "Needs change record" tag and moving MR out of draft.

Is it possible to add the #[AutowireServiceClosure(...)] attribute and delete create() here?

There needs to be a separate issue to support that. It was on my todo list to create one, but I haven't gotten around to it yet and thought it would be best to land this and #3544994: Make service closures serializable in classes using DependencySerializationTrait first.

Also, does the CR need before and after examples?

For the class's constructor arguments deprecation? I'm not sure how that would go.

Added the BC for the class properties. I genericized the __get(), but then didn't do a trait because the properties aren't really deprecated, since we'll be bringing them back via property hooks in D12 with minimum PHP 8.4 support in #3544903: [meta] Use property hooks to instantiate services from service closures. So the __get() will only be a thing for this short window of time.

Stubbed the follow up #3554337: [PP-1] Support the AutowireServiceClosure attribute in AutowireTrait to inject service closures per #14.

Thanks for the reviews.
I wondering now if this should be postponed on #3544994: Make service closures serializable in classes using DependencySerializationTrait?

Blocking on #3544994: Make service closures serializable in classes using DependencySerializationTrait.