-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
BaseService - Use lowercase key in resetSingle #5596
BaseService - Use lowercase key in resetSingle #5596
Conversation
Can you add test case to prove the code works?
|
55fa455
to
98b03ba
Compare
@kenjis test is added |
tests/_support/Config/Services.php
Outdated
* | ||
* @return stdClass | ||
*/ | ||
public static function someService($getShared = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our convention is that a service name is all lower case.
This should be someservice
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know about that convention but there is nothing limiting the user not to use any letter casing.
Function names in PHP are case insensitive so it will work with any case. And it make sense to me to use camel case for service names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be lowercase.
CodeIgniter4/system/Config/Services.php
Line 180 in 812af1e
public static function curlrequest(array $options = [], ?ResponseInterface $response = null, ?App $config = null, bool $getShared = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding a test!
I added some inline comments.
I think the added someService()
should be removed,
because it is not necessary for the test.
Use an existing service.
98b03ba
to
8207e27
Compare
Ok I made this very simple now. I though we should have some real case example with non lowercase name and that is the reason why I added But since there is a convention for lowercase in service method there is no need for that. We just need to make the method do to lowercase conversion to match how other methods in Service classes behave. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
As you say, a user may use camelCase,
but I think this test case is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since serviceExists()
uses lowercase only for method lookup I think all services must be lower unless the user is calling them directly from Config\Services
.
This is a good catch and fix.
Since injectMock converts key to lowercase we need to use lowercase key as well in resetSingle method
Description
Fixes #5595
Checklist: