[ResourceBundle][WIP] Global settings #1168
Conversation
| @@ -38,6 +38,8 @@ public function load(array $config, ContainerBuilder $container) | |||
|
|
|||
| $classes = isset($config['resources']) ? $config['resources'] : array(); | |||
|
|
|||
| $container->setParameter('sylius.defaultsettings', $config['settings']); | |||
There was a problem hiding this comment.
defaultsettings => default_settings
There was a problem hiding this comment.
Or even: sylius.settings.default
There was a problem hiding this comment.
I like sylius.settings.default
There was a problem hiding this comment.
@Arn0d But I guess this could be confusing, so I would recommend to call it: sylius.settings.pagination or sylius.resource.settings
Mostly because we use sylius.settings.xxx for SettingsBundle stuff.
There was a problem hiding this comment.
sylius.resource.settings sounds good !
| * | ||
| * @var array | ||
| */ | ||
| protected $defaultSettings; |
There was a problem hiding this comment.
I don't like this name $defaultSettings... These are the settings we can configure right via config.yml right ? Default sounds like we can not override them. What about $crudSettings ?
There was a problem hiding this comment.
Why not $resourcesSettings ??
There was a problem hiding this comment.
I don't like both proposal :D! What about settings?
There was a problem hiding this comment.
$settings is good enough IMO.
There was a problem hiding this comment.
Same for all other variables with $default in names.
|
I am back guys ! |
| ->arrayNode('paginate') | ||
| ->addDefaultsIfNotSet() | ||
| ->children() | ||
| ->booleanNode('active')->defaultTrue()->end() |
There was a problem hiding this comment.
Instead of active, you should IMO use: canBeEnabled()/canBeDisabled().
There was a problem hiding this comment.
I will check it out !
|
@Sylius/core-team / @stloyd , the build is green. What do you think ? |
| * | ||
| * @return array | ||
| */ | ||
| private function mergeDefaultSettings(array $parameters, array $settings, $reset) |
There was a problem hiding this comment.
Why do we need this method? Passing a boolean parameter here, just to do something or do nothing does not seem right...
There was a problem hiding this comment.
I merge the settings with the route parameters. The both configuration have not the same structure. It is not the best place for this, perhaps you should play the kernel event.
There was a problem hiding this comment.
Something like :
class KernelControllerSubscriber {
public function onKernelRequest(GetResponseEvent $event)
{
// If resquest has _sylius attribute, we parse it and create a Configuration object and put it the container.
}Like that, we can get the Configuration object everywhere (controller, twig extension, etc...).
|
@pjedrzejewski Do you prefer it? |
| */ | ||
| public function setParameters(array $parameters) | ||
| { | ||
| $this->parameters = $parameters; |
|
ping @stloyd / @Sylius/core-team |
|
"your test run exceeded 50.0 minutes" for PHP 5.3 and 5.5, Green for 5.4. ping @pjedrzejewski |
| { | ||
| $limit = $this->get('limit', 10); | ||
| return (boolean) $this->parameters->get('limit', $this->settings['limit']['enabled']); |
There was a problem hiding this comment.
I guess you should use has() here, as you force to boolean anyway.
There was a problem hiding this comment.
Heuuu, no I just tested. I don't want to check if the parameter exists but I want to check its value because limit is always set to false or true.
|
I restarted the build, now it is green ! |
|
@pjedrzejewski I need to rebase it but can you give me feedback, please? |
| ->arrayNode('sorting') | ||
| ->canBeEnabled() | ||
| ->children() | ||
| ->scalarNode('parameter_name')->defaultNull()->end() |
|
I just don't get why this has to be that complex... Isn't defining configuration and merging 2 arrays enough to have defaults? This is huge... |
|
Yes you're right I will simplify the configuration definition. Did you have a look at the rest of PR ? |
|
@pjedrzejewski and now? Do you prefer it ? |
|
ping @Sylius/core-team |
|
@pjedrzejewski : Can you give me feedback plz? If you don't like it we can close it ! I explain it a bit more (in my first post). |
|
@pjedrzejewski friendly ping ? |
|
ping @pjedrzejewski! Do I rebase it? Is there any chances to get this PR merged? |
|
whoo. i'd like something like this :) |
|
👍 |
1 similar comment
|
👍 |
|
@Arn0d Yes, looks good to me, I will merge as soon as you rebase. Thanks! |
|
@pjedrzejewski done :) ! |
[ResourceBundle][WIP] Global settings
|
Thanks Arnaud! |
Fixing the parameter parser (see #1168)
Related RFC #1126
The configuration of each route is used in several places (controller and twig extension for example), so we needed to parse it and merge the default settings several time. But I wanted to it only one time and inject it in the container. It is done in the
KernelControllerSubscriber. I created a new class namedParameterwhich extendsParameterBag. This class is declared as a service and use byResourceExtensionandConfiguration.