All injected services should be marked readonly

The only exception is if a service can be injected using a setter, but AFAIK AU/PM don't do that anywhere.

Not quite true. StageBase uses LoggerAware(Trait|Interface), setting it to new NullLogger() initially. Services that extend StageBase have a setLogger() call. That said, the property they're setting comes from LoggerAwareTrait and is not read-only, so there's no problem here. Just thought I'd point that out for completeness' sake. 🤓

Nice catch! :)

I have one question along with the review, why can't we mark services in \Drupal\automatic_updates\Form\UpdaterFormM and \Drupal\automatic_updates\Form\UpdateReady readonly.

99% of the way there! 👍

Did a very nitpicky review of the first half (up to package_manager/src/Validator/MultisiteValidator.php), and most of my remarks are about reformatting for legibility, or switching from protected to private since we're touching these lines already anyway.

#7: good question! You can easily find the answer yourself though: in any of the failing Functional tests, do a var_dump($this->getSession->getPage()→getContent()); and you'll see the exact PHP error.

I made all the @internal class parameters private, should I revert them back or make the class final?

Looked over everything and I think it's good. There are just a couple of things I think should be reverted, but then it's RTBC.

Sorry for the confusion before; I had missed Wim's comment about making protected properties of internal classes into private properties. I agree with him.

🚢

I ❤️ read-only properties, and now Automatic Updates ❤️s them too.

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