Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

mpajunen
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

The function and property name changes in preparation of #14330 to allow using any callable with config builders.

@mpajunen mpajunen mentioned this pull request Apr 15, 2015
4 tasks
@mpajunen mpajunen changed the title [Config] Use callable instead of \Closure [Config] Renamings to allow all callback types Apr 15, 2015
@mpajunen
Copy link
Contributor Author

ping @stof (or someone else), any more thoughts on this?

protected $normalizationClosures = array();
protected $finalValidationClosures = array();
protected $normalizationCallbacks = array();
protected $finalValidationCallbacks = array();
Copy link
Member

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Referenced aliases added.

Copy link
Member

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

Copy link
Contributor Author

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.

@GromNaN
Copy link
Member

GromNaN commented May 21, 2015

What is the benefit of renaming the method and property ?

@mpajunen
Copy link
Contributor Author

There is no immediate benefit, this PR is simply a preparation step for #14330 that allows any callable to be used instead of just \Closures. The current names would be confusing then.

@mpajunen mpajunen force-pushed the config-callbacks branch 3 times, most recently from 0a06989 to 8e9f489 Compare May 21, 2015 20:36
*/
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);
Copy link
Member

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

Copy link
Contributor Author

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.

// Backwards compatibility for old property names, to be removed in 3.0
$this->normalizationClosures = &$this->normalizationCallbacks;
$this->finalValidationClosures = &$this->finalValidationCallbacks;
Copy link
Member

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.

Copy link
Contributor Author

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.

@fabpot fabpot added the Config label Oct 5, 2015
@fabpot
Copy link
Member

fabpot commented Oct 5, 2015

Closing this PR as the other one was closed as well. Supporting closure only in this case seems enough.

@fabpot fabpot closed this Oct 5, 2015
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.

5 participants