-
Notifications
You must be signed in to change notification settings - Fork 379
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
use setFactory service definition method for symfony >= 2.6 (when possible) #566
Changes from all commits
f332505
23425b5
6d4e0c8
6c6ca23
dc58bd9
491a9dd
6764a83
cf9d22d
80b99ea
8599396
61e2c1e
8d14b43
2c2e235
658148d
ce1ade3
d7d06d5
4c577ac
c217913
0177d6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
use Symfony\Component\Form\AbstractType; | ||
use Symfony\Component\Form\FormInterface; | ||
use Symfony\Component\Form\FormView; | ||
use Symfony\Component\OptionsResolver\OptionsResolver; | ||
use Symfony\Component\OptionsResolver\OptionsResolverInterface; | ||
|
||
/** | ||
|
@@ -14,6 +15,9 @@ | |
*/ | ||
class ImageType extends AbstractType | ||
{ | ||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function buildView(FormView $view, FormInterface $form, array $options) | ||
{ | ||
$view->vars['image_path'] = $options['image_path']; | ||
|
@@ -24,7 +28,10 @@ public function buildView(FormView $view, FormInterface $form, array $options) | |
$view->vars['link_attr'] = $options['link_attr']; | ||
} | ||
|
||
public function setDefaultOptions(OptionsResolverInterface $resolver) | ||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function configureOptions(OptionsResolver $resolver) | ||
{ | ||
$resolver->setRequired(array( | ||
'image_path', | ||
|
@@ -39,11 +46,25 @@ public function setDefaultOptions(OptionsResolverInterface $resolver) | |
)); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function setDefaultOptions(OptionsResolverInterface $resolver) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docblock |
||
{ | ||
$this->configureOptions($resolver); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getParent() | ||
{ | ||
return 'file'; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getName() | ||
{ | ||
return 'liip_imagine_image'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
use Liip\ImagineBundle\Binary\BinaryInterface; | ||
use Symfony\Component\OptionsResolver\OptionsResolver; | ||
use Symfony\Component\OptionsResolver\OptionsResolverInterface; | ||
use Symfony\Component\HttpKernel\Kernel; | ||
|
||
class CacheResolver implements ResolverInterface | ||
{ | ||
|
@@ -46,7 +47,7 @@ public function __construct(Cache $cache, ResolverInterface $cacheResolver, arra | |
$optionsResolver = new OptionsResolver(); | ||
} | ||
|
||
$this->setDefaultOptions($optionsResolver); | ||
$this->configureOptions($optionsResolver); | ||
$this->options = $optionsResolver->resolve($options); | ||
} | ||
|
||
|
@@ -222,18 +223,32 @@ protected function saveToCache($cacheKey, $content) | |
return false; | ||
} | ||
|
||
protected function setDefaultOptions(OptionsResolverInterface $resolver) | ||
protected function configureOptions(OptionsResolver $resolver) | ||
{ | ||
$resolver->setDefaults(array( | ||
'global_prefix' => 'liip_imagine.resolver_cache', | ||
'prefix' => get_class($this->resolver), | ||
'index_key' => 'index', | ||
)); | ||
|
||
$resolver->setAllowedTypes(array( | ||
'global_prefix' => 'string', | ||
'prefix' => 'string', | ||
'index_key' => 'string', | ||
)); | ||
$allowedTypesList = array( | ||
'global_prefix' => 'string', | ||
'prefix' => 'string', | ||
'index_key' => 'string', | ||
); | ||
|
||
if (version_compare(Kernel::VERSION_ID, '20600') >= 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the logic behind this change. Looks strange to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @makasim Since symfony 2.6 setAllowedTypes api changed. Now you call it with option and array of types instead off array where key is option and value is allowed types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah okay There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but it is a BC break, isn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but it's deprecated since 2.6 to be removed in 3.0 https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/OptionsResolver/OptionsResolver.php#L591 |
||
foreach ($allowedTypesList as $option => $allowedTypes) { | ||
$resolver->setAllowedTypes($option, $allowedTypes); | ||
} | ||
} else { | ||
$resolver->setAllowedTypes($allowedTypesList); | ||
} | ||
|
||
} | ||
|
||
protected function setDefaultOptions(OptionsResolverInterface $resolver) | ||
{ | ||
$this->configureOptions($resolver); | ||
} | ||
} |
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.
IMO, it is better to check symfony version here to be more explicit why we have this
if
.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.
And it will also reflect the preferred may and the one which will be removed in future.
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.
@makasim I was referring how it was resolved in other bundles like https://github.com/doctrine/DoctrineBundle/blob/master/DependencyInjection/DoctrineExtension.php#L136 for example
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.
okay, could you add a comment to
else
statement that it can be remove when we drop support of symfony <2.7