[regression] The new property \Drupal\Core\Form\ConfigFormBase::$typedConfigManager conflicts with some contrib modules

That search yielded mostly false positives. Also restricting to *Form.php produces far fewer: 49 instead of 588.

Actually, even that produces a lot of false positives.

If we also require ConfigFormBase to be present, then it drops to 15 matches.

That … makes it a bit more unclear.

I propose:

1. Rename core's use (prefix with an underscore?).
2. Trigger a deprecation error that warns this will no longer be allowed in Drupal 11.
3. … but we'll need to be careful to avoid the opposite problem when we eventually make this a reality, to avoid subclasses relying on the renamed property instead of intended property … 😬 So … not entirely sure actually 😅

That seems a bit less concerning 👍 Updated the smallest list to the issue summary.

Could we mark the new property private and deprecate warning if there's existing property that is not the right type? Something along the lines of this.

I think that is a net improvement! 👍

Devel also broke with this change and it is not listed in your report so not sure the report is super accurate - https://gitlab.com/drupalspoons/devel/-/issues/483. Devel code is also on git.drupal.org so it is eligible.

I cleaned up the proposed change from #3 and pushed it to a MR.

Marking so it can still make 10.2 window, and if it breaks contrib modules.

But should a follow up be opened for test coverage?

@smustgrave How could we test this?

@lauriii What about the concern in #2.3?

@Wim Leers I changed the property to a private property to try to address that. It's not perfect because you need to override the __construct if you want to use it. Not sure if there is a perfect solution for this other than trying to fix all of contrib.

You could install one of the contributed modules to see if there's a fatal error. I used Layout Paragraphs myself.

devel

AFAICT Devel's form is just incorrect:

  /**
   * Constructs a new SettingsForm object.
   *
   * @param \Drupal\devel\DevelDumperPluginManagerInterface $devel_dumper_manager
   *   Devel Dumper Plugin Manager.
   */
  public function __construct(DevelDumperPluginManagerInterface $devel_dumper_manager) {
    $this->dumperManager = $devel_dumper_manager;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('plugin.manager.devel_dumper')
    );
  }

— https://gitlab.com/drupalspoons/devel/-/blob/7aeb5e2a20ac4186525a496e131...

Note how it's not calling the parent constructor. That means the first required argument for the ConfigFormBase base class has also been missing for the past decade: ConfigFactory $config_factory.

layout_paragraphs

https://git.drupalcode.org/project/layout_paragraphs/-/blob/2.0.x/src/Fo... also defines

  protected $typedConfigManager;

This is what conflicts.

Simply making ConfigFormBase's private is sufficient.

Distilled

Distilled down to the essence:

<?php

// before, in 10.1
class BaseOld {
    public function __construct() {}
    public function printTest(): void {
        print $this->test;
    }
}
// after
class BaseNew {
    private bool $test;
    public function __construct(?bool $test = NULL) {
        $test = $test ?? TRUE;
        $this->test = $test;
    }
    public function printTest(): void {
        print $this->test;
    }
}

// NOTE: IDENTICAL!
class FooOld extends BaseOld {
    protected bool $test;
    public function __construct() {
        parent::__construct();
        $this->test = FALSE;
    }
}
class FooNew extends BaseNew {
    protected bool $test;
    public function __construct() {
        parent::__construct();
        $this->test = FALSE;
    }
}

$f = new FooOld();
$f->printTest();
$f = new FooNew();
$f->printTest();

— https://3v4l.org/UBUSh

So in 10.1 ConfigFormBase did not strictly require the parent constructor to be called, because there is a helper getter for the config factory in FormBase:

  protected function configFactory() {
    if (!$this->configFactory) {
      $this->configFactory = $this->container()->get('config.factory');
    }
    return $this->configFactory;
  }

Wonder if we should add a similar helper for the typed config manager service?

I like that proposal, @longwave. It's pragmatic, fixes the current disruption in 10.2.0-beta1 and prevents future disruption.

Marking to get +1 from either (and hopefully both) @catch and @lauriii.

Yeah that sounds worth doing to me.

No remarks.

This should solve the problem for contrib and we can clean it up further in 11.x. Getting this in now so we fix the regression in 10.2.0-rc1.

Committed and pushed a9865eb372 to 11.x and 1142669363 to 10.2.x. Thanks!

Will also update and publish the change record.

CR updated 👍

Thanks for the fix.

FYI I just changed Devel's create() method so it avoids this sort of problem and avoids a PHPStan complaint as well - https://gitlab.com/drupalspoons/devel/-/merge_requests/164.

👍

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