Skip to content
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 ability to configure default menu options #278

Open
wants to merge 1 commit into
base: master
from

Conversation

@wouterj
Copy link
Collaborator

commented Aug 27, 2015

In the CMF, menu's aren't created programatically. With the current code, it means the menu options needs to be saved in the database in order to be set on the menu items.

It would be much better if we could configure this in the application settings. This PR implements a very simple DefaultOptionsExtensions, setting the default options based on a configured set of options.

This extension is automatically registered as soon as at least one option is configured.

@stof @Nek- I would love to hear your opinion about this (both the idea and whether it should be in this bundle or in the CMF).

Related issue: symfony-cmf/menu-bundle#128

@wouterj wouterj modified the milestone: 2.1 Aug 27, 2015

use Knp\Menu\ItemInterface;
/**
* An extension to dynamically configure default menu options.

This comment has been minimized.

Copy link
@dbu

dbu Aug 28, 2015

Collaborator

why "dynamically"? if the default options are in the configuration file, i would consider that rather static.

When creating a menu, menu options are set by passing them to ``addChild()``.
When all menu items need the same option, adding the same options to each child
can be cumbersome. In some cases, passing options isn't possible (e.g. when the
menu was created based on a database).

This comment has been minimized.

Copy link
@dbu

dbu Aug 28, 2015

Collaborator

i think the last line is not needed - and a bit confusing. i can store the options in the database and have them passed.

i think the point is really avoiding redundancy or having the effort to carry around default values in your application (that you probably set up in configuration anyways). in the cmf effort its the redundancy of storing options with every node which is an even worse redundancy.

@dbu

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2015

i like it!

xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

<parameters>
<parameter key="knp_menu.extension.default_options.class">Knp\Bundle\MenuBundle\Extension\DefaultOptionsExtension</parameter>

This comment has been minimized.

Copy link
@stof

stof Aug 28, 2015

Collaborator

please don't introduce new class parameters. this practice is now discouraged in symfony

This comment has been minimized.

Copy link
@wouterj

wouterj Aug 28, 2015

Author Collaborator

Ok, in the CMF, we decided to add them in the 1.x serie, for consistency and remove them all in 2.0. So I was a bit unsure about the standard of KnpMenu :)

@wouterj wouterj force-pushed the wouterj:configure_default_values branch from d9514b6 to 72dc946 Aug 29, 2015

@wouterj

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2015

updated

@dbu

This comment has been minimized.

Copy link
Collaborator

commented Aug 29, 2015

👍

@Nek- do you agree with this one?

@@ -38,6 +38,11 @@ public function load(array $configs, ContainerBuilder $container)
}
$container->setParameter('knp_menu.default_renderer', $config['default_renderer']);
if (isset($config['default_menu_options'])) {

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2015

Collaborator

it is always set (prototyped array nodes have a default value of empty array)

@dbu

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2015

oh, right. otherwise do you agree with the PR @stof or is that the only issue that needs to be fixed?

*/
public function buildOptions(array $options = array())
{
return array_merge($this->options, $options);

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2015

Collaborator

many options of the library are arrays. This makes them hard to use with your extension

This comment has been minimized.

Copy link
@wouterj

wouterj Sep 20, 2015

Author Collaborator

So you suggest using array_merge_recursive instead?

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2015

Collaborator

I don't know what to use here. array_merge_recursive has a very weird behavior when associate arrays are involved, so it is not the solution either (it would be even worse). array_replace_recursive would already be a bit better as we mostly have associative arrays (but this one leads to an unexpected behavior when lists are merged together though, and this happens in extras, for instance for the list of matched routes)

# app/config/config.yml
knp_menu:
default_menu_options:
class: menu-item

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2015

Collaborator

there is no class option in KnpMenu. Please use a real use case in the documentation (which will also help you see that the current implementation is not very usage)

@dbu

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2015

this was in the 2.1 milestone but @stof just tagged 2.1.0

should we remove the milestone now? does it go to 2.2?

@stof

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2015

Well, I don't rely on the milestone in the project actually. I won't delay a release because a new feature is not ready. It can simply go in a new release.

@stof stof removed this from the 2.1 milestone Sep 28, 2015

@dbu

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2016

can we wrap this one up somehow?

@garak

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

@wouterj is this PR still active?
Some comments need to be addressed, and some conflicts need to be solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.