-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Deprecate Pool param for SearchHandler #6308
Deprecate Pool param for SearchHandler #6308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplification in the tests feels good :)
Shouldnt you change the xml config? |
c11bf8f
to
363dc61
Compare
I forgot. I don't use conf anymore on my project. |
*/ | ||
public function __construct(Pool $pool, $caseSensitive = true) | ||
public function __construct($deprecatedPoolOrCaseSensitive, $caseSensitive = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should add = true as default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous next major comment was
NEXT_MAJOR: remove default true value for $caseSensitive and add bool type hint.
So I didn't add the default true value. But I don't know why we didn't want a default value indeed.
EDIT: the default value was remove in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well then lets leave it without default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! The Pool is only a service locator for admins? Maybe we can use Symfony directly for that. Just to think about it, nothing to change here
It could be great indeed to improve/remove the Pool. For instance there is a lot of There is also a |
This one could be related with the issue we had on the Pr that adds support for sf5 on master. Not having a template registry service locator made a block on that PR |
Subject
I am targeting this branch, because BC.
See #6307 (comment)
Changelog