-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Add configureSettings to block service #204
Add configureSettings to block service #204
Conversation
I have to check against built-in services. |
$service = $this->blockService->get($block); | ||
$service->setDefaultSettings($optionsResolver, $block); | ||
|
||
$reflector = new \ReflectionMethod($service, 'setDefaultSettings'); |
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.
I think this should be compute once with a CompilerPass to avoid performance impact on using the reflection API.
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.
How a CompilerPass
will help to detect and throw deprecation errors?
I did the same method as Symfony. Please see: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ResolvedFormType.php#L201-L207
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 can store an array with the result of the reflection and still send a trigger. Or at least store the result for one service to avoid doing the same over and over if a Page is composed of the same block.
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.
To be honest, I have some difficulties to see what you want.
Can you maybe propose a PR on this one to show me?
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.
Something like this.
$this->reflectionCache = [
'class\path' => ['isOldOverwritten' => true|false, 'isNewOverwritten' => true|false
];
$class = get_class($service);
if ($this->reflectionCache[$class]['isOldOverwritten'] && !$this->reflectionCache[$class]['isNewOverwritten']) {
trigger_error('The Sonata\BlockBundle\Block\BlockServiceInterface::setDefaultSettings() method is deprecated since version 2.3 and will be removed in 3.0. Use configureSettings() instead. This method will be added to the BlockServiceInterface with SonataBlockBundle 3.0.', 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.
The reflectionCache cache attribute can either be compute once at with a CompilerPass or at runtime.
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.
Ok I'll try. 👍
@rande Reflection cache is on the place. 👍 |
Did you plan to add unit test ? |
@rande Unit tests are already here for those classes. I'm missing something? |
AbstractBlockService::configureSettings fixes Symfony deprecation issue. BlockServiceInterface::setDefaultSettings is now deprecated.
Tests updated to fix deprecation notice errors. AFAIK, the coverage still the same. |
Add configureSettings to block service
* | ||
* @var array | ||
*/ | ||
private $reflectionCache; |
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.
There is no need to cache reflection objects. See this benchmark: https://gist.github.com/mindplay-dk/3359812
@soullivaneuh PR sent for all the remaining open issues |
AbstractBlockService::configureSettings
fixes Symfony deprecation issue.BlockServiceInterface::setDefaultSettings
is now deprecated.Fixes #201
@rande Please don't merge it now! :-)