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

Fix BaseConfig didn't load Registrar files properly #1947

Merged
merged 2 commits into from
Apr 17, 2019
Merged

Fix BaseConfig didn't load Registrar files properly #1947

merged 2 commits into from
Apr 17, 2019

Conversation

vibbow
Copy link
Contributor

@vibbow vibbow commented Apr 16, 2019

Description
In origional BaseConfig.php file, in registrProperties function, it use locator to find all Config/Registrar.php files in known namespace, and it returned all file path as an array.

In following code, it use the file path as a callable object, to check if registrars exists (by using method_exists) for current config file, which didn't work in my case.

I'm not sure if method_exists works with file path, but I updated this function to load all registrars files first before future usage.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@vibbow
Copy link
Contributor Author

vibbow commented Apr 16, 2019

Wait a second, the tests is failed and it's related to this change.

In theory, it should discovery Registrar file at test/_support/Config/Registrar.php, and calling RegistrarConfig function, and the function did return an array.

Need to figure out why following code get triggered

if (! is_array($properties))
{
	throw new \RuntimeException('Registrars must return an array of properties and their values.');
}

@vibbow
Copy link
Contributor Author

vibbow commented Apr 16, 2019

Get failed test fixed.

@lonnieezell lonnieezell merged commit 8592ee1 into codeigniter4:develop Apr 17, 2019
@vibbow vibbow deleted the pr_fix_config_registrar branch April 17, 2019 04:06
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