[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 😅

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.

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?

1. devel

AFAICT Devel's form is just incorrect:

<?php

class SomeService {
    public string $something;
}

// before, in 10.1
class BaseOld {
    protected SomeService $test;
    public function __construct(?SomeService $test = NULL) {
        if ($test === NULL) {
            $test = new SomeService();
            $test->something = 'fallback';
        }
        $this->test = $test;
    }
    public function printTest(): void {
        print __CLASS__ . "\t" . $this->test->something . "\n";
    }
}
// after
class BaseNew {
    private SomeService $test;
    public function __construct(?SomeService $test = NULL) {
        if ($test === NULL) {
            $test = new SomeService();
            $test->something = 'fallback';
        }
        $this->test = $test;
    }
    public function printTest(): void {
        print __CLASS__ . "\t" . $this->test->something . "\n";
    }
}

//NOTE: IDENTICAL!
// class FooOld extends BaseOld {
//     protected $test;
//     public function __construct() {
//         parent::__construct();
//         $this->test = new SomeService();
//         $this->test->something = 'subclass';
//     }
// }
class FooNew extends BaseNew {
    protected $test;
    public function __construct() {
        parent::__construct();
        $this->test = new SomeService();
        $this->test->something = 'subclass';
    }
}

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

— https://3v4l.org/m6AJL

So in 10.1, subclasses of 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?

Also makes me wonder if forms should use setter injection for base dependencies (config factory, typed config manager, anything new we want to add in the future) and constructor injection for specific dependencies for individual forms?

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

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

Yeah that sounds worth doing to me.

As this is a regression for contrib we should try to resolve it before 10.2.0-rc1 (this week) if possible.

Added the helpful getter method, and changed the constructor deprecation so that it allows anything, but issues a deprecation if you pass in something that isn't TypedConfigManagerInterface.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

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.

👍