Uh oh!
There was an error while loading. Please reload this page.
feat(IAppConfig): Add rector to migrate IConfig calls to IAppConfig - #78
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Joas Schilling <coding@schilljs.com>
Uh oh!
There was an error while loading. Please reload this page.
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
provokateurin
commented
Apr 27, 2026
And what happens if IConfig is also used for reading system values and not just app values? |
CarlSchwan
commented
Apr 27, 2026
According to the |
nickvergessen
commented
Apr 27, 2026
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. |
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