Skip to content

BaseService - Use lowercase key in resetSingle - #5596

Merged
MGatner merged 1 commit into
codeigniter4:developfrom
najdanovicivan:services-resetSingle
Jan 23, 2022
Merged

BaseService - Use lowercase key in resetSingle#5596
MGatner merged 1 commit into
codeigniter4:developfrom
najdanovicivan:services-resetSingle

Conversation

@najdanovicivan

Copy link
Copy Markdown
Contributor

Since injectMock converts key to lowercase we need to use lowercase key as well in resetSingle method

Description
Fixes #5595

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 tests needed Pull requests that need tests bug Verified issues on the current code behavior or pull requests that will fix them labels Jan 20, 2022
@kenjis

kenjis commented Jan 20, 2022

Copy link
Copy Markdown
Member

Can you add test case to prove the code works?

Note, we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

@najdanovicivan
najdanovicivan force-pushed the services-resetSingle branch 2 times, most recently from 55fa455 to 98b03ba Compare January 21, 2022 14:35
@najdanovicivan

Copy link
Copy Markdown
Contributor Author

@kenjis test is added

Comment thread tests/_support/Config/Services.php Outdated
Comment thread tests/_support/Config/Services.php Outdated
Comment thread tests/_support/Config/Services.php Outdated
*
* @return stdClass
*/
public static function someService($getShared = true)

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This must be lowercase.

public static function curlrequest(array $options = [], ?ResponseInterface $response = null, ?App $config = null, bool $getShared = true)

Comment thread tests/_support/Config/Services.php Outdated
Comment thread system/Config/BaseService.php Outdated

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

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.

@kenjis kenjis removed the tests needed Pull requests that need tests label Jan 22, 2022
@najdanovicivan

Copy link
Copy Markdown
Contributor Author

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 someService()

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.

@najdanovicivan
najdanovicivan requested a review from kenjis January 22, 2022 10:23

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

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.

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

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.

@MGatner
MGatner merged commit f1054c2 into codeigniter4:develop Jan 23, 2022
@najdanovicivan
najdanovicivan deleted the services-resetSingle branch March 23, 2022 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: BaseService injectMock resetSingle case sensitivity

4 participants