Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Lock] Add LockFactoryInterface#39634

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
Nyholm wants to merge7 commits intosymfony:5.xfromNyholm:lock-factory-interface

Conversation

@Nyholm
Copy link
Member

QA
Branch?5.x
Bug fix?no
New feature?yes
Deprecations?yes
Tickets
LicenseMIT
Doc PR

I saw that the LockFactory does not have an interface. And the RateLimit component has a config option to "Add your own lock factory".

This PR includes a BC break to our experimental RateLimiter component. But it is a super minor so I am okey with it.

@jderusse
Copy link
Member

no strong opinion about this. I created a LockFactoryInyerface in the original PR (#21093) But removed afterwards (I don't remember who suggested it. Probably@nicolas-grekas if I remember well...)

The factory is pretty simple and I never needed another implementation or decoration. Do you have a use-case in mind?

@Nyholm
Copy link
MemberAuthor

The rate limiter provides bundle config like:

framework:rate_limiter:acme:lock_factory:# Any service that is an instance of LockFactory

It does not make much sense to have a config value where you pretty much have only one valid config.

Same thing with the constructor ofRateLimiterFactory. The third argument should be null or an object of theLockFactory class. The dependency inversion principle says that it should be an interface instead.

So no, apart from these more or less academical reasons, I dont have a use-case.

@jderusse
Copy link
Member

It does not make much sense to have a config value where you pretty much have only one valid config.

hmm, the factory take a store in argument. You may have instance of the Factory in the DIC.

framework:    lock:        foo: redis://localhost        bar: flock

provides 2 factories

@Nyholm
Copy link
MemberAuthor

Yes, sure, you may have multiple lock factories which all would be different objects of LockFactory. But that is still missing my point of dependency inversion.

@nicolas-grekasnicolas-grekas added this to the5.x milestoneDec 27, 2020
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

No issue on my side.
We should deprecate the LockFactory alias and add one for the new interface also.

@Nyholm
Copy link
MemberAuthor

The PR is updated. There is a lot of deprecated service names and aliases.

Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

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

I've never felt I needed a different implementation of the lock factory. The factory is just a tiny bit of glue code around the store. I don't oppose this change, but I think the current way of not having an interface is just fine.

@Nyholm
Copy link
MemberAuthor

I've never felt I needed a different implementation of the lock factory. The factory is just a tiny bit of glue code around the store. I don't oppose this change, but I think the current way of not having an interface is just fine.

I agree that this change is mostly academical.

derrabus reacted with thumbs up emoji

@derrabus
Copy link
Member

By the way, I think we can remove the "BC Break" label. You've changed the constructor signature in a compatible way and the lock factory is assigned to a private variable and is never exposed.

Nyholm reacted with thumbs up emoji

@fabpot
Copy link
Member

I think it was me not wanting this interface and I haven't changed my mind. "academic" reasons are probably the worst :)

derrabus reacted with thumbs up emojiNyholm reacted with laugh emoji

@Nyholm
Copy link
MemberAuthor

"academic" reasons are probably the worst :)

Hm... I disagree with that.

Thank you for reviewing and for the feedback.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@jderussejderussejderusse approved these changes

@derrabusderrabusderrabus left review comments

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

6 participants

@Nyholm@jderusse@derrabus@fabpot@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp