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.

Sign up for GitHub

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

Jump to bottom

Infinite loop when fetching some cache-related services via container in projects without registered config service #286

Merged
merged 11 commits into from
Jan 19, 2024
Merged

Infinite loop when fetching some cache-related services via container in projects without registered config service #286

merged 11 commits into from
Jan 19, 2024

Conversation

InvisibleSmiley
Copy link

@InvisibleSmiley InvisibleSmiley commented Jan 17, 2024 •
edited by boesing
Loading

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

Add regression test for issue #284

@jhatlak
Add regression test for issue 284
9ae9f5c 
Signed-off-by: Jens Hatlak <jens.hatlak@check24.de>
@Ocramius Ocramius mentioned this pull request Jan 17, 2024
Closed
@boesing
qa: replace ContainerTest with ConfigProviderIntegrationTest
d2b0e92 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
qa: optimize config provider integration test for better type-inference
77e351a 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
bugfix: allow deprecated storage factory configuration check to opera…
e474463 
…te on projects without project configuration

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
bugfix: preserve config cache name
766573c 
This fixes an infinite loop in combination with `laminas-servicemanager` as outlined in laminas#284

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing added the Bug Something isn't working label Jan 17, 2024
@boesing boesing added this to the 3.12.1 milestone Jan 17, 2024
@boesing boesing changed the title Add regression test for issue 284 Infinite loop when fetching some cache-related services via container in projects without registered config service Jan 17, 2024
@boesing boesing linked an issue Jan 17, 2024 that may be closed by this pull request
ServiceManager::get results in endless loop since 3.11.0 #284
Closed
@boesing boesing requested a review from froschdesign January 17, 2024 14:35
boesing
boesing approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

LGTM :trollface:

@boesing
qa: add unit test to verify that for the config service, `canCreate…
411151e 
…` method is not calling container

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
gsteel
gsteel reviewed Jan 17, 2024
View reviewed changes
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Spotted a typo in the docs, and also the constant name PRESERVED_ should be RESERVED_ right?

docs/book/v3/application-integration/usage-in-a-laminas-mvc-application.md Outdated Show resolved Hide resolved
src/Service/StorageCacheAbstractServiceFactory.php Outdated Show resolved Hide resolved
src/Service/StorageCacheAbstractServiceFactory.php Outdated Show resolved Hide resolved
@boesing
qa: rephrase exception message
2643615 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
docs: fix some typo regarding project configuration
0872a58 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
qa: fix type, there is a difference between preserved and reserved
f965101 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
qa: rename assertion method and its argument to reflect actual assertion
bc01ee8 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
docs: use markdown admonitions/callouts
734d78e 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Xerkus
Xerkus reviewed Jan 18, 2024
View reviewed changes
src/Service/StorageCacheAbstractServiceFactory.php
@@ -14,7 +14,8 @@
*/
class StorageCacheAbstractServiceFactory implements AbstractFactoryInterface
{
public const CACHES_CONFIGURATION_KEY = 'caches';
public const CACHES_CONFIGURATION_KEY = 'caches';
private const RESERVED_CONFIG_SERVICE_NAME = 'config';
Copy link
Member

Choose a reason for hiding this comment

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

Why introduce constant? Lines 76 and 81 are not using it.

Copy link
Member

Choose a reason for hiding this comment

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

because its way more readable than just having a random string somewhere.
The get does not need to check for the requested name as it shouldn't get passed due to the canCreate method returning false (implicit check).

I did a quick lookup with phpdbg and from what I can see, having this constant does not even produce more opcodes than using string here.

So IMHO, due to readability and same opcode amount, it should be fine having this around.

Xerkus
Xerkus reviewed Jan 18, 2024
View reviewed changes
docs/book/v3/application-integration/usage-in-a-laminas-mvc-application.md
Comment on lines -40 to +44
The factory `Laminas\Cache\Service\StorageCacheAbstractServiceFactory` uses the configuration, searches for the configuration key `caches` and creates the storage adapters using the discovered configuration.
The factory `Laminas\Cache\Service\StorageCacheAbstractServiceFactory` uses the configuration, searches for the configuration key `caches` and creates the storage adapters using the discovered configuration.

WARNING: **Cache Named `config` Is Not Possible**
A cache named `config` is not possible due to internal service conflicts with MVC configuration.
The service named `config` is reserved for project configuration and thus cannot be used with the `caches` configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Cache name must be unique identifier in container. Not just config but any conflicting name is not possible.

Copy link
Member

Choose a reason for hiding this comment

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

yah but thats usually up to the user while it is not for config as we now do have explicit check within the abstract static factory.
So I think this might be good enough for now. We still should tackle this specific issue within laminas-servicemanager.

@boesing boesing merged commit bf8bc7f into laminas:3.12.x Jan 19, 2024
16 checks passed
@github-actions github-actions bot mentioned this pull request Jan 19, 2024
Merged
@boesing boesing mentioned this pull request Jan 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsteel gsteel gsteel left review comments

@boesing boesing boesing approved these changes

@Xerkus Xerkus Xerkus left review comments

@froschdesign froschdesign froschdesign left review comments

Assignees
No one assigned
Labels
Bug Something isn't working
Projects
None yet
Milestone
3.12.1
Development

Successfully merging this pull request may close these issues.

ServiceManager::get results in endless loop since 3.11.0
6 participants
@InvisibleSmiley @froschdesign @Xerkus @boesing @gsteel @jhatlak