Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Fix phpstan codeigniter.configArgumentInstanceof #9390

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

neznaika0
Copy link
Contributor

Description
Modules does not extend BaseConfig. The object must be created directly by new Modules()

$modules instanceof Modules it is necessary, otherwise rector tries to overwrite __constructor(). For comparison with null, there are also rules - a change is acceptable here

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@samsonasik samsonasik requested a review from kenjis January 9, 2025 16:26
@paulbalandan paulbalandan added the refactor Pull requests that refactor code label Jan 10, 2025
@michalsn
Copy link
Member

Shouldn't we allow Module config to be used with config()?

@neznaika0
Copy link
Contributor Author

Modules do not extend BaseConfig and cannot have a Registrar. This is the same configuration as Autoloader, Paths. + they are not cached

@michalsn
Copy link
Member

michalsn commented Jan 10, 2025

Yes, I know, but we're not talking about caching or registrars. This is a question, should the Module config have a single instance through Factories, as it is now? IMO yes.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem like no one else is sharing my concern, so let's go.

@neznaika0
Copy link
Contributor Author

Perhaps this is a problem for modular sites, who used config('Modules'). But this system files...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants