feat(IAppConfig): Add rector to migrate IConfig calls to IAppConfig#78
feat(IAppConfig): Add rector to migrate IConfig calls to IAppConfig#78nickvergessen merged 1 commit intomainfrom
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Joas Schilling <coding@schilljs.com>
nickvergessen
requested review from
CarlSchwan,
ChristophWurst and
come-nc
and removed request for
CarlSchwan
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
- How does that behave when IAppConfig is already injected in the class?
- Does it removes IConfig injection if it is unused afterwards? Or do we have another rector that removes unused injected services?
- I have a preference for the scoped IAppConfig but I suppose this is way harder to automate with rector.
| foreach ($propertyNames as $propertyName) { | ||
| if ($this->isName($propertyFetch->name, $propertyName)) { |
There was a problem hiding this comment.
isNames should be used instead, like above?
There was a problem hiding this comment.
Right, not sure how I missed that
| private function makeUniquePropertyName(ClassMethod $constructor, string $desired): string | ||
| { | ||
| $taken = []; | ||
| foreach ($constructor->getParams() as $param) { | ||
| $name = $this->getName($param->var); | ||
| if (is_string($name)) { | ||
| $taken[$name] = true; | ||
| } | ||
| } | ||
| if (!isset($taken[$desired])) { | ||
| return $desired; | ||
| } | ||
| $i = 2; | ||
| while (isset($taken[$desired . $i])) { | ||
| $i++; | ||
| } | ||
|
|
||
| return $desired . $i; | ||
| } |
There was a problem hiding this comment.
Looks like it will inject the same class a second time rather than re-using the same property.
There was a problem hiding this comment.
well only when referencing another class, in case that was not clear to you.
When its already there it's used. I added new tests in #81 to show that
And what happens if IConfig is also used for reading system values and not just app values? |
According to the |
No, because it could be used with other direct methods that are not replacable or with IUserConfig which is not rectored yet.
That was basically exactly my thought.
I also did not use the IAppConfig from AppFramework which I personally prefer as it is limited to your own app and avoids mistakes. But that would be more breaking and I don't think rector rules should have a risk. |
5 participants
AI (if applicable)
Description
Added a rector to replace all IConfig::App() calls with their respective IAppConfig counterpart
Motivation and context
Cleaner code
How has this been tested?
Types of changes
PR checklist