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

Config Property Types #6214

Merged
merged 9 commits into from
Jul 4, 2022
Merged

Config Property Types #6214

merged 9 commits into from
Jul 4, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 1, 2022

Description
Supersedes #6189

  • add types in app/Config files
  • update MockConfig
  • update test Config vaules
  • remove tests that are no longer needed
  • update admin/starter/app/Config/Paths.php

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

@kenjis kenjis added 4.3 breaking change Pull requests that may break existing functionalities labels Jul 1, 2022
@samsonasik
Copy link
Member

admin appstarter seems needs update too

public $systemDirectory = __DIR__ . '/../../vendor/codeigniter4/framework/system';

@kenjis
Copy link
Member Author

kenjis commented Jul 1, 2022

@samsonasik Good catch! Thanks.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Wow thanks @kenjis this looks great! So many PHPStan ignores cleaned up, that feels good.

Since the breaking change is entirely confined to mock classes I am fine with documenting that and proceeding. I think the likelihood of developers extending our mock classes is very slim, and it would only be in a test context. These classes probably should have been final to begin with.

Approval conditional on updated documentation.

@kenjis
Copy link
Member Author

kenjis commented Jul 1, 2022

@MGatner The changes of the Mock Config classes break existing tests.
The property types do not match unless developers update app/Config files manually.
https://3v4l.org/tnpIv

But as you say,

it would only be in a test context.

So I would like to proceed to add the docs.

Comment on lines 8 to 13
// Load the system's routing file first, so that the app and ENVIRONMENT
// can override as needed.
if (is_file(SYSTEMPATH . 'Config/Routes.php')) {
require SYSTEMPATH . 'Config/Routes.php';
}

Copy link
Member Author

Choose a reason for hiding this comment

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

These lines should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@kenjis
Copy link
Member Author

kenjis commented Jul 2, 2022

Added User Guide.

@kenjis kenjis merged commit ad65846 into codeigniter4:4.3 Jul 4, 2022
@kenjis kenjis deleted the config-types branch July 4, 2022 07:11
@kenjis kenjis mentioned this pull request Jul 4, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants