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

Make Pool immutable #6735

Merged
merged 2 commits into from
Jan 9, 2021
Merged

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Jan 3, 2021

Subject

Ref: #6709

Pool works as an Admin registry and its state is not needed to be changed once is instantiated, this PR will make Pool immutable in 4.0.

Also Pool mocks used in tests have been replaced by instances of Pool (the remaining ones are supposed to be removed in 4.x).

I am targeting this branch, because these changes are BC.

Closes #6709.

Changelog

### Deprecated
- Deprecated `Pool::setAdminServiceIds` in favor of passing service ids as argument 2 to the constructor.
- Deprecated `Pool::setAdminGroups` in favor of passing service ids as argument 3 to the constructor.
- Deprecated `Pool::setAdminClasses` in favor of passing service ids as argument 4 to the constructor.

@franmomu franmomu added the minor label Jan 3, 2021
Comment on lines 97 to 99
$pool->replaceArgument(1, $config['title']);
$pool->replaceArgument(2, $config['title_logo']);
$pool->replaceArgument(3, $config['options']);
Copy link
Member

Choose a reason for hiding this comment

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

You're replacing argument 1, 2 and 3 in both AddDependencyCallsCompilerPass.php and here but with different values.

Shouldn't these lines be removed in favor of the setDeprecatedPropertiesForBC call ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left them intentionally just in case some weird BC break, just to be safe, but I guess they can be removed.

VincentLanglet
VincentLanglet previously approved these changes Jan 5, 2021
@VincentLanglet VincentLanglet requested a review from a team January 5, 2021 18:34
@SonataCI
Copy link
Collaborator

SonataCI commented Jan 6, 2021

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

SonataCI commented Jan 8, 2021

Could you please rebase your PR and fix merge conflicts?

VincentLanglet
VincentLanglet previously approved these changes Jan 8, 2021
@VincentLanglet VincentLanglet requested a review from a team January 8, 2021 20:57
@SonataCI
Copy link
Collaborator

SonataCI commented Jan 8, 2021

Could you please rebase your PR and fix merge conflicts?

Setters have been deprecated in favor of passing the needed arguments
to the constructor.
@VincentLanglet VincentLanglet requested a review from a team January 8, 2021 21:52
@OskarStark OskarStark merged commit 87c0ff9 into sonata-project:3.x Jan 9, 2021
@franmomu franmomu deleted the make_pool_immutable branch January 9, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Pool immutable
4 participants