[9.5.x backport] $this->configFactory is not set properly in the constructor of \Drupal\system\Form\CronForm

Other approach - form using config() method from trait, so it just missing to assign the property defined in parent class

It looks cleaner

it looks like bug so could be backported

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Reviewing patch #5 looks like a good simple change. Guess it was a small phpstan error.

Feel good to mark.

PHP 8.1 property promotion would have avoided this bug! That's out of scope, though...

I agree that the approach in #5 is cleaner.

I reviewed the parent class and confirmed that FormBase already includes documentation for the corresponding protected property:

  /**                                                                           
   * The config factory.                                                        
   *                                                                            
   * Subclasses should use the self::config() method, which may be overridden to                                                                               
   * address specific needs when loading config, rather than this property      
   * directly. See \Drupal\Core\Form\ConfigFormBase::config() for an example of 
   * this.                                                                      
   *                                                                            
   * @var \Drupal\Core\Config\ConfigFactoryInterface                            
   */
  protected $configFactory;

That's interesting and gave me pause for a moment, but it's also probably why no one noticed this bug before PHPStan.

The only hypothetical issue I could think of here would be if someone were relying on the unintended behavior of a null CronForm::$configFactory, but that's extremely obscure and also against the above documented best practice, so I think this fix is also safe to backport.

Trying to commit this but PHPStan is OOMing on me, so clicking save on my review comment in case I forget about this issue.

I closed the MR for clarity.

I'm having PHPStan issues with committing this. First it was OOMing; now, I'm getting:

Running PHPStan on *all* files.
 -- ------------------------------------------------------------------------- 
     Error                                                                    
 -- ------------------------------------------------------------------------- 
     Reached internal errors count limit of 50, exiting...                    
     Internal error: Child process timed out after 600.0 seconds. Try making  
     it longer with parallel.processTimeout setting.                          
 -- ------------------------------------------------------------------------- 


 [ERROR] Found 2 errors                                                         


PHPStan: failed

If #5 is the RTBCed patch, then I think this is practically a duplicate of #3323994: Fix PHPStan L1 errors "Constructor of class Foo has an unused parameter $bar".

Okay sheesh. I was finally able to commit this by:

1. cp core/phpstan.neon.dist core/phpstan.neon
2. Editing that file and adding:
   

    parameters:
    
   +  parallel:
   +    maximumNumberOfProcesses: 8
   +
      level: 1
   

Then the full scan completed after a mere hour and twenty minutes (!!).

Apologies @mondrake; didn't see your comment before I pushed, because of the hour-and-twenty-minute thing.

Thanks everyone for working this. Setting PTBP for backport after the commit freeze, since this is technically a runtime bug.

Maybe silly question, but why running PHPStan on the entire code base locally before committing? Aren't DrupalCI runs enough? Are you running all the PHPUnit tests locally as well? (that would require what... one day?)

@mondrake, I'm forced to because the baseline is changing. On normal commits it runs only on changed files.

Queued test runs against 10.0.x and 9.5.x in the hopes that this can be cherry-picked safely.

Cherry-picked to 10.0.x. NW for a 9.5.x backport, which will give me the chance to try @catch's suggestion of trying a lower number of parallel processes when committing the baseline change...

9.5 has no phpstan so only fix

Patch #22 applied successfully on drupal version 9.5.x and its working fine.
Thank you

#22 is good to go.

@gaurav-mathur, thank you for reviewing this issue!

The automated testing infrastructure tells us whether the change set still applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

What we do need people to review is whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information. When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

Also see the issue credit guidelines for more information on which kinds of contributions are credited.

In this case, I confirmed that the change was identical to the original patch, minus the change to the phpstan baseline because there is no phpstan in 9.5.x

Committed #22 to 9.5.x. Thanks!

Published the CR.

...Actually, I deleted the CR, because it was about the previous approach with the deprecation.