Skip to content

Conversation

@MGatner
Copy link
Member

@MGatner MGatner commented Jun 27, 2022

Description
Applies non-breaking property types to Config files in App. Applied by Rector.

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

@MGatner MGatner changed the base branch from develop to 4.3 June 27, 2022 17:31
@MGatner
Copy link
Member Author

MGatner commented Jun 27, 2022

This should all be safe to change with the possible exception of Email which has some odd initial definitions.

@MGatner
Copy link
Member Author

MGatner commented Jun 27, 2022

Actually this may not work because of the class extensions in CodeIgniter\Test\Mock config classes. I have to drop this for now but will think on that later.

@kenjis
Copy link
Member

kenjis commented Jun 28, 2022

Fatal error: Type of CodeIgniter\Test\Mock\MockCLIConfig::$baseURL must be string (as in class Config\App) in /home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/Mock/MockCLIConfig.php on line 40

@MGatner
Copy link
Member Author

MGatner commented Jun 28, 2022

I saw that, there are a few other Configs in there that break as well. If we want to move forward with this I think the approach would be to remove the properties from the Mock Config files and use their constructors to change the values that need changing. That way they would still work with whatever is in a user's app/Config folder, typed or not.

@kenjis kenjis added the 4.3 label Jul 1, 2022
@kenjis kenjis mentioned this pull request Jul 1, 2022
5 tasks
@kenjis
Copy link
Member

kenjis commented Jul 1, 2022

It would be impossible to ensure that an existing app or tests would never break
because a Type Error would occur if the configuration class were inherited.

@MGatner
Copy link
Member Author

MGatner commented Jul 1, 2022

But that is only if the developer manually updates their Config folder with the new files. This should be safe with the caveat of these Mock classes, which we can fix internally but can't guarantee that extensions don't exist.

@kenjis
Copy link
Member

kenjis commented Jul 1, 2022

But that is only if the developer manually updates their Config folder with the new files.

Yes, you are correct!

@kenjis
Copy link
Member

kenjis commented Jul 4, 2022

#6214 was merged.

@kenjis kenjis closed this Jul 4, 2022
@MGatner MGatner deleted the config-types branch July 4, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants