-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Config] Renamings to allow all callback types #14364
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
Conversation
0921e97
to
872a79f
Compare
872a79f
to
a4d278d
Compare
ping @stof (or someone else), any more thoughts on this? |
protected $normalizationClosures = array(); | ||
protected $finalValidationClosures = array(); | ||
protected $normalizationCallbacks = array(); | ||
protected $finalValidationCallbacks = array(); |
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.
Aliasing the old ones by reference would allow preserving BC.
you should do it in the constructor, and for completeness also add a magic __clone method:
public function __construct(...)
{
$this->normalizationClosures = &$this->normalizationCallbacks;
$this->finalValidationClosures = &$this->finalValidationCallbacks;
// ...
}
// ...
public function __clone()
{
// Break cross-clones references but preserve them inside each one.
unset($this->normalizationCallbacks, $this->finalValidationCallbacks);
$this->normalizationCallbacks = $this->normalizationClosures;
$this->finalValidationCallbacks = $this->finalValidationClosures;
$this->normalizationClosures = &$this->normalizationCallbacks;
$this->finalValidationClosures = &$this->finalValidationCallbacks;
}
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.
Thanks. Referenced aliases added.
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.
You forgot to keep the declaration above
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.
Works without, but of course it makes sense to keep the old property declarations as well. Restored now, with deprecation notes.
What is the benefit of renaming the method and property ? |
There is no immediate benefit, this PR is simply a preparation step for #14330 that allows any |
0a06989
to
8e9f489
Compare
*/ | ||
public function setNormalizationClosures(array $closures) | ||
{ | ||
$this->normalizationClosures = $closures; | ||
trigger_error('The '.__METHOD__.' method is deprecated since version 2.8 and will be removed in 3.0. Use the setNormalizationCallbacks() method instead.', E_USER_DEPRECATED); |
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.
Please add @
in front of trigger_error: @trigger_error(...
, same below
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.
Done.
Replace references to closures in function, property and parameter names in preparation of future callable support.
8e9f489
to
bfa965c
Compare
|
||
// Backwards compatibility for old property names, to be removed in 3.0 | ||
$this->normalizationClosures = &$this->normalizationCallbacks; | ||
$this->finalValidationClosures = &$this->finalValidationCallbacks; |
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.
Would it be an option to remove the normalizationCallbacks
and finalValidationCallbacks
properties and implementing __get()
and __set()
for them instead? This way we could also trigger deprecation notices when the deprecated properties are used.
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.
Yes, using magic methods would allow allow deprecation notices and also somewhat simpler implementation (no need for __clone()
tricks). It would also make the properties effectively public, which could be a problem. In addition the new properties could also probably be private, unless there's some specific reason the old ones were protected.
All of these changes look like bikeshedding to me though. I checked with Packanalyst earlier and found just one class that would need to be changed for 3.0 compatibility -- and that one uses the setter methods. A quick search is not going to find every use case, but I think using the properties directly is likely rare even in custom subclasses.
Of course, I can still make the changes if there's agreement that using __get()
and __set()
would be preferred.
Closing this PR as the other one was closed as well. Supporting closure only in this case seems enough. |
The function and property name changes in preparation of #14330 to allow using any callable with config builders.