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

#434 Enable Lazy service for phpredis #440

Merged
merged 1 commit into from
Jul 18, 2018
Merged

#434 Enable Lazy service for phpredis #440

merged 1 commit into from
Jul 18, 2018

Conversation

B-Galati
Copy link
Collaborator

The service will be lazy if symfony/proxy-manager-bridge is installed
As a consequence it will allow to warmup the cache even if Redis server is not reachable

fix #434

@B-Galati
Copy link
Collaborator Author

B-Galati commented Jun 30, 2018

@dkarlovi Would you be able to test it out ? (or you can just edit your vendors with this little change)

@B-Galati
Copy link
Collaborator Author

I guess I should also edit the doc about symfony/proxy-manager-bridge and phpredis behavior

The service will be lazy if symfony/proxy-manager-bridge is installed
As a consequence it will allow to warmup the cache even if Redis server is not reachable

fix snc#434
@B-Galati
Copy link
Collaborator Author

B-Galati commented Jul 1, 2018

Doc updated

@dkarlovi
Copy link

dkarlovi commented Jul 2, 2018

@B-Galati I'll test it tomorrow. Would this be the correct approach?

@B-Galati
Copy link
Collaborator Author

B-Galati commented Jul 2, 2018

@dkarlovi What do you wean exactly ?

From my knowledge, I do think lazy service is a good solution as it's something easy, powerful and well supported by Symfony.

@dkarlovi
Copy link

dkarlovi commented Jul 2, 2018

@B-Galati I mean, would making the service lazy have the desired behaviour and/or bad consequences elsewhere?

From discussions at Symfony's Slack, lazy=true is almost considered an anti-pattern and (some) users called for the feature to be completely deprecated by Symfony.

@B-Galati
Copy link
Collaborator Author

B-Galati commented Jul 2, 2018

@dkarlovi Thank you. Can you give the reasons behind this thinking? What would be the alternatives?

I am not aware about any bad consequences for lazy services. Certainly we gonna lose some very little performances in some situation but that's all I can see. In other situation where for example, the redis client is not used like the cache warmup, performances will be better.

IMHO lazy services are just an infrastructure detail.

Perhaps I am wrong so please let me know for anything.

@OskarStark
Copy link

Symfony just decided to make all doctrine listeners lazy by default in 4.2

@dkarlovi
Copy link

dkarlovi commented Jul 6, 2018

@B-Galati sorry for the delay. This does indeed fix #434 (as in, allows the container to be prebuilt). There doesn't seem to be any side-effects that are noticeable to me.

About lazy services, this might have been a "storm in a teacup" by a group of loud users, but the prominent opinion was lazy services are almost always a bad idea. This might be considered an exception. :)

Thank you for working on this!

@B-Galati
Copy link
Collaborator Author

B-Galati commented Jul 6, 2018

@dkarlovi Thanks for the feedback :-)

Basically PR #424 is doing the same thing but the implementation is different.

@curry684 curry684 merged commit a2d0faa into snc:2.1 Jul 18, 2018
@dkarlovi
Copy link

@curry684 I see you've merged this, would you consider a bugfix release?

@curry684
Copy link
Collaborator

curry684 pushed a commit that referenced this pull request Jul 19, 2018
The service will be lazy if symfony/proxy-manager-bridge is installed
As a consequence it will allow to warmup the cache even if Redis server is not reachable

fix #434

(cherry picked from commit a2d0faa)
@vchebotarev vchebotarev mentioned this pull request Sep 16, 2018
@piotrkochan
Copy link
Contributor

Lazy loading for this service makes PHP 5.6 fail because of invalid cached Redis class. See #465

rvanlaak added a commit to rvanlaak/SncRedisBundle that referenced this pull request Dec 11, 2018
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.

5 participants