Skip to content

[FrameworkBundle] Missing Changelog for deprecating services #38584

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

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

jderusse
Copy link
Member

Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? yes
Tickets /
License MIT
Doc PR /

I forgot to add CHANGELOG/UPGRADE when deprecating alias lock in #38576. (thank you @nicolas-grekas for reminding this)

This PR also adds a missing alias registered for argument.

@nicolas-grekas
Copy link
Member

Now I'm wondering: do we want to deprecate named locks? Why?
The original issue was only about the default lock.

@jderusse
Copy link
Member Author

I believe the issue #38458 is about removing Lock in favor of LockFactory and not only the default one. /cc @lyrixx

I think arguments pointed in the issue are valid for both default lock and named locks. We should deprecate all of theme because they are dangerous/confusing for end users (no concurrency between services). And easy for end-user to fix:

  • inject the factory and call ->createLock() in the constructor
  • or create the service themselve $services->set(LockInterface::class)->factory(LockFactory::class, 'createLock')->args(['default'])

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

I agree with @jderusse 👍🏼

@derrabus
Copy link
Member

Thank you Jérémy.

@derrabus derrabus merged commit ab7f489 into symfony:5.x Oct 15, 2020
@jderusse jderusse deleted the deprecate-lock-service-changelog branch October 25, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants