DefaultSingleLazyPluginCollection::setConfiguration() accepts NULL but ConfigurableInterface::setConfiguration() does not

IMHO this is such a simple change (purely to do with function signatures) that test coverage is excessive here?

PHPStan gets in the way once again. Locally, PHPStan does not complain 😬

Tried an alternative implementation.

I don't know why it passed locally, but it's probably trusting the phpdoc which probably says it's always an array and never null.

Thanks, @mglaman, that makes sense!

Note that an alternative solution could be to modify
\Drupal\Core\Config\Entity\ConfigEntityBase::set():

  public function set($property_name, $value) {
    if ($this instanceof EntityWithPluginCollectionInterface) {
      $plugin_collections = $this->getPluginCollections();
      if (isset($plugin_collections[$property_name])) {
        // If external code updates the settings, pass it along to the plugin.
        $plugin_collections[$property_name]->setConfiguration($value);
      }
    }
…

👆 We could make it not call ::setConfiguration() if $value is NULL.

I read null as reset/no config. So I'd transform null to [] and still call. Otherwise the intent to remove or clear configuration wouldn't happen and existing configuration would persist.

The idea that null === remove.

If I'm understanding correctly believe the suggestion is to revert

https://git.drupalcode.org/project/drupal/-/merge_requests/5540/diffs?co...

Right?

Done.

This looks good, but I think it could use a comment. Also, don't we need test coverage for this?

NW for the inline comment.

in #4 was mentioned test coverage may be overkill but if there's valid concern then test coverage should be added too.

I'm not sure about the solution chosen here, should we deprecate calling DefaultSingleLazyPluginCollection::setConfiguration() with NULL, and attempt to make it typehinted in drupal 11?

@phenaproxima: Test coverage added. The test-only job fails 👍

@borisson_: Done. Turns out it is simple: https://3v4l.org/dcCF0 👍 CR: https://www.drupal.org/node/3406191 →

Looks great now.

One small question about adding a future-proof type hint.

AFAICT PHPStan does not allow us to do that. See MR for PHPStan output.

What the hell is PHPStan talking about??? That is contravariant.

From https://www.php.net/manual/en/language.oop5.variance.php (my emphasis):

Covariance allows a child's method to return a more specific type than the return type of its parent's method. Whereas, contravariance allows a parameter type to be less specific in a child method, than that of its parent.

Oh, I see -- PHPStan is annoyed because of the direction of the contravariance, since this MR doesn't change \Drupal\Component\Plugin\LazyPluginCollection::setConfiguration.

But then I gotta wonder...why aren't we also changing that method too? The doc comment says that $configuration needs to be an array...

But then I gotta wonder...why aren't we also changing that method too? The doc comment says that $configuration needs to be an array.

Because the end goal is public function setConfiguration(array $configuration), without the ?.

And if we set it to ?array now, then it's impossible to change the typehint to array in 11.

array works, ?array does not work

Yeah, that makes sense. It's too bad, but in light of the discovery about LazyPluginCollection, I think what we have now is a correct approach. In Drupal 11 we can fix it all properly, array $configuration everywhere!

Adjusting credit and restoring RTBC.

Yay!

Asked a question / have a suggestion on the MR.

See #15 and #21 for 3v4l.org examples for why the current typehint additions are AFAICT fine and @phenaproxima's last MR comment is AFAICT correct.

What are we missing?

AFAICT everything is actually addressed:

1. I've applied Alex' language suggestions.
2. The remarks Alex posted were apparently on suggestions by @phenaproxima that I already explained why they could not work.

So: back to RTBC. Alex mentioned in #24 a question was asked on the MR, but I don't see any question, only responses on _suggestion threads_ that were responded to here in the issue? 🤔

Committed 871bc1a and pushed to 11.x. Thanks!

Published the CR

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