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 new service replacement service provider precedence on core factory implementations #4593

Merged

Conversation

element-code
Copy link
Contributor

@element-code element-code commented Apr 21, 2021

This fixes #4483
Replacing static::foobar() with \Config\Services::foobar() (https://regex101.com/r/QyahlU/1).

Checklist:

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

@element-code
Copy link
Contributor Author

@MGatner what about static::getSharedInstance() calls, should they be replaced too?

@MGatner
Copy link
Member

MGatner commented Apr 21, 2021

The calls themselves are okay since all instances will be stored in BaseServices which App and Core both extend. However, the call out in BaseServices::getSharedInstance() needs to be changed I think:

static::$instances[$key] = static::$key(...$params);

...otherwise when Core fails to match an instance and calls static it won't check App unless we update that line.

@MGatner
Copy link
Member

MGatner commented Apr 21, 2021

Also, if you could find a way to add tests that confirm the correct instance is loaded I think that would be a great addition.

@element-code
Copy link
Contributor Author

element-code commented Apr 21, 2021

@MGatner sadly I'm not able to do this (adding tests) by my own right now.

@MGatner
Copy link
Member

MGatner commented Apr 22, 2021

No worries, these tests were already lacking and that's not your fault. I will take a look at them later. For now can you change BaseService::getSharedInstance() to use AppServices? then I think we will be ready.

@element-code
Copy link
Contributor Author

@MGatner it's done 👍

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.

Sorry for the delay! I thought I submitted this but I see this morning that it is "pending". One small style change and we're golden!

system/Config/BaseService.php Outdated Show resolved Hide resolved
@MGatner MGatner force-pushed the fix-replace-core-services branch from 07b3f0c to 4339db0 Compare May 16, 2021 20:40
@element-code
Copy link
Contributor Author

element-code commented May 17, 2021

Thanks 👍
Sadly I can't find time for this

@paulbalandan paulbalandan merged commit 889de06 into codeigniter4:develop May 17, 2021
@element-code element-code deleted the fix-replace-core-services branch August 2, 2021 20:34
@khodakhah
Copy link

khodakhah commented Oct 22, 2021

Hi,

Before this change, it was possible to create different Service classes to categorize the services. A developer only needed to create a new class (inherit from CodeIgniter\Config\BaseService) in Config/ directory.

But after this, getSharedInstance() is not working in any service class but Services.php 🙄

Why you didn't write self::getSharedInstance() instead of BaseServices::getSharedInstance()?

Thanks

@element-code
Copy link
Contributor Author

element-code commented Oct 28, 2021

Basically because this would ignore the service provider precedence, see #4483
Calling static:: would mean that CodeIgniter4\Config\Services would basically ignore all overwritten services in App\Config\Services.

If youre against this behavior, you can easily extend CodeIgniter\Config\BaseService, overwrite getSharedInstance with your custom logic (e.g. calling static::$name()) and let your service-provider classes extend from your custom class.

@MGatner @paulbalandan what about (auto) service-provider discovery? Anything planned in that direction?
There is some logic in

protected static function discoverServices(string $name, array $arguments)
which seems to be never used.. ^^

@khodakhah
Copy link

Thank you @element-code for your answer. 👍

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.

Bug: New Service replacement fails at service provider precedence on core factory implementations
4 participants