avanzu / AdminThemeBundle

Admin Theme based on the AdminLTE Template for easy integration into symfony
MIT License
281 stars 149 forks source link

Unrecognized option "knp_menu" under "avanzu_admin_theme" #180

Closed numerogeek closed 6 years ago

numerogeek commented 6 years ago

Hello, I've got trouble when using the base_layout.

I tried to override it, and it tells me :

Method "knp_menu" for object "Avanzu\AdminThemeBundle\Helper\ContextHelper" does not exist in app/Resources/AvanzuAdminThemeBundle/views/layout/base-layout.html.twig

I tried to add the knp_menu.enabled in the config but it says : Unrecognized option "knp_menu" under "avanzu_admin_theme"

I think that the twig globals are not passed to my view override. Any idea ?

thanks !

shakaran commented 6 years ago

@numerogeek are you using master or 1.3 version? Could you copy your current config and base-layout override to see if there are something different?

benr77 commented 6 years ago

This is since upgrading to the latest beta 7 release. I am suffering from the same issue.

benr77 commented 6 years ago

I had to downgrade back to 2.0.0-beta.5 to get my configuration to be accepted. Are the configuration changes documented anywhere?

shakaran commented 6 years ago

@numerogeek @benr77 I push a fix https://github.com/avanzu/AdminThemeBundle/commit/a6afac02b91022a7e27bd98ea0562e1383a6022d related to ContextHelper config in master, please test if you can so this should be fixed

benr77 commented 6 years ago

It doesn't seem to have fixed my problem:

In my composer.json

"avanzu/admin-theme-bundle": "dev-master#a6afac02b91022a7e27bd98ea0562e1383a6022d"

then composer update avanzu/admin-theme-bundle

I get the following error

[Symfony\Component\Config\Definition\Exception\InvalidConfigurationException]  
  Unrecognized options "knp_menu, theme" under "avanzu_admin_theme"

If I revert back to 2.0.0-beta.5 it works again.

Here is my configuration

avanzu_admin_theme:
  knp_menu:
    enable : true
  theme:
    default_avatar: bundles/avanzuadmintheme/img/avatar.png
    skin: skin-blue  # see skin listing for viable options
    fixed_layout: false      # -------------------------------------------------------
    boxed_layout: false      # these settings relate directly to the "Layout Options"
    collapsed_sidebar: false      # described in the adminlte documentation
    mini_sidebar: false      # -------------------------------------------------------
    control_sidebar: true      # controls whether the right hand panel will be rendered
    widget:
      collapsible: false
      removable: false
      solid: true
      use_footer: false
shakaran commented 6 years ago

@benr77 thanks for your testing! I push new updates in master, please test if you can for receive your new feedback

numerogeek commented 6 years ago

@shakaran Sounds good for knp_menu but if you want to customize the skin it does not work. In fact, in the ContextHelper, you set the default in : $resolver->setDefaults([ 'use_twig' => true, 'use_assetic' => true, 'options' => [], 'skin' => 'skin-blue',

But in the Configuration, you put the skin option under the "theme" node.

When I try to change the skin:

avanzu_admin_theme: theme: skin: skin-black

it does not change anything and {{ dump(avanzu_admin_context.option) }} gives me an empty array.

shakaran commented 6 years ago

@numerogeek the skin param don't should appear under _avanzu_admincontext.option, if should be under _avanzu_admincontext.skin directly

avanzu_admin_theme:
    theme:
        skin: skin-black

Take in mind that _avanzu_admincontext is a shorcut to class _avanzu_admin_theme.contexthelper

All under the "context" should be present:

{{ dump(avanzu_admin_context.skin) }}

Another case could be via the twig function "body_class" which use some default layouts in the bundle templates:

body_class({'skin': 'skin-black'})

The body_class has more params like _fixed_layout, boxed_layout, collapsed_sidebar, minisidebar . The full config for that is:

body_class({
    'skin': 'skin-black',
    'fixed_layout': 'true',
    'boxed_layout': 'true',
    'collapsed_sidebar': 'true',
    'mini_sidebar': 'true',
})
numerogeek commented 6 years ago

@shakaran When I try to dump {{ dump(avanzu_admin_context.skin) }} I got

Method "skin" for object "Avanzu\AdminThemeBundle\Helper\ContextHelper" does not exist

numerogeek commented 6 years ago

@shakaran I think that the configuration is overwritten with default value when the container builds.

numerogeek commented 6 years ago

@shakaran Update: In the AvanzuAdminExtension class :

public function load(array $configs, ContainerBuilder $container)
{
        $baseConfiguration = new Configuration();

        // Load the configuration from files
        try
        {
            $configs = $container->getExtensionConfig($this->getAlias());
        }
        catch(InvalidConfigurationException $e)
        {
            echo 'AvanzuAdminBundle:' . $e->getMessage() . PHP_EOL;
            $configs = [];
        }
....

$configs is resetted with the default value. Why do you do this $configs = $container->getExtensionConfig($this->getAlias()); ?

shakaran commented 6 years ago

@numerogeek before to this fixes, when you have a invalid config value set (for example an invented key or unexistant). You will get a exception that will avoid render anything.

In your case, it seems that you get a InvalidConfigurationException, that means that your config has some invalid value, and you should see a message with "AvanzuAdminBundle" string in console or printed.

The problem here, is that it should fallback to base config, which I set a $config = [] array value, and probably it should fallback to base configuration instead.

The $container->getExtensionConfig($this->getAlias()); is needed to fetch and validate the current config before to apply or read the values in config. That means, set a base config, override with the values set by the user and after apply the final result.

So, please watch if you are receiving a message with AvanzuAdminBundle previously with the invalid configuration. I will push a fix for the base configuration.

numerogeek commented 6 years ago

@shakaran Here is my config:

avanzu_admin_theme:
    bower_bin: %bower_bin% # that's the default value
    use_assetic: true

To reproduce: Try to clear your cache, set a custom bower_bin path (I need it for travis because travis don't store it in /usr/local/bin) and try to print the bower path selected by the load configuration.

I suspect that's exactly the same for the skin option etc...

shakaran commented 6 years ago

@numerogeek do you have defined the parameter %bower_bin% ? Are you sure that it is valid without quotes?

I mean:

parameters:
    bower_bin: "your custom bower bin for travis"

avanzu_admin_theme:
    bower_bin: "%bower_bin%" # See the quoting here
    use_assetic: true

Probably the problem come from the invalid parameter or the missing quote of the parameter.

Make a composer update to master, and watch in the update, if you get a warning line starting with the text "AvanzuAdminBundle: invalid config"

numerogeek commented 6 years ago

I tried using xDebug: I don't fall in the catch. My config is valid. Have you tried locally ? (don't forget to clear the cache before)

As I said in comment in the PR, I doubt that the $container->getExtensionConfig($this->getAlias()); parses anything.

Check the function content :

    public function getExtensionConfig($name)
    {
        if (!isset($this->extensionConfigs[$name])) {
            $this->extensionConfigs[$name] = array();
        }

        return $this->extensionConfigs[$name];
    }

Please consider reopen my PR.

shakaran commented 6 years ago

About the catch. it was set in getExtensionConfig() instead processConfiguration() so you didn't get the warning message properly. Already pushed that change (in that part you have reason, so my apologies since I misstakenly believe that was throwing in getExtensionConfig() instead processConfiguration() ).

Even if I thank you your effort to fix it (I always appreciate help fixing things or improving this), unfortunally, your PR don't solve the problem in the right spot, because the config has to be fetch first from getExtensionConfig(), that value is used for processConfiguration as second argument, that will trigger the InvalidConfiguration if there are one (note, trigger by processConfiguration)

The validation will be improved (note: using validateConfiguration()) when I get a stable and 0 issues of this bundle (that will be after 2.0 release, I expect):

See for example other bundles use this:

        $extension = $this->findExtension($name);

        $extensionAlias = $extension->getAlias();
        $configs = $container->getExtensionConfig($extensionAlias);
        $configuration = $extension->getConfiguration($configs, $container);

        $this->validateConfiguration($extension, $configuration);

        $configs = $container->resolveEnvPlaceholders($container->getParameterBag()->resolveValue($configs));
numerogeek commented 6 years ago

@shakaran Thank you for your feedback. Could you please show me which bundle implements this kind of configuration ? Because I don't see where it loads the user's configuration ?

shakaran commented 6 years ago

@numerogeek just var_dump() or use xDebug for see the values and you will see the data received in each part of the config bundle.

About the bundles using that, just grep some vendors in your symfony installation by getExtensionConfig() for example: /vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ConfigDebugCommand.php

I notice that the problem was that load() method get the config from an initial array parameter, and prepend needs preload the config array. So I think that now should fix your issue with latest commits. Please let me know with latest master.

If a invalid config is set, for example:

avanzu_admin_theme:
    example: true

The warning should be clear like:

AvanzuAdminBundle: invalid config (prepend): Unrecognized option "example" under "avanzu_admin_theme"
    The config options for the bundle AvanzuAdminBundle were skipped
AvanzuAdminBundle: invalid config (load): Unrecognized option "example" under "avanzu_admin_theme"
    The config options for the bundle AvanzuAdminBundle were skipped
shakaran commented 6 years ago

@numerogeek I push more fixes, please let me know when you can test again if it is resolved this issue

numerogeek commented 6 years ago

it's now good for me. Thank you.

shakaran commented 6 years ago

@numerogeek thanks! Happy to see that it is already resolved. Please if you can check other issues where I am waiting your feedback (help to test other issues still opened would help me a lot too). So we can release soon a stable release in benefit for all

kussberg commented 6 years ago

I have the same error for KNP integration, i tried both to put it directly under "avanzu_admin_theme" and also under "avanzu_admin_theme.options" both of them say "Unrecognized option "..." under "avanzu_admin_theme":

"name": "avanzu/admin-theme-bundle",
            "version": "1.3.8",
            "source": {
                "type": "git",
                "url": "https://github.com/avanzu/AdminThemeBundle.git",
                "reference": "ed9a11f4c002bbc542adb680bddc5222e11e85a6"
            },
            "dist": {
                "type": "zip",
                "url": "https://api.github.com/repos/avanzu/AdminThemeBundle/zipball/ed9a11f4c002bbc542adb680bddc5222e11e85a6",
                "reference": "ed9a11f4c002bbc542adb680bddc5222e11e85a6",
                "shasum": ""
            },

Here is my config:

# Avanzu Admin Theme Bundle
avanzu_admin_theme:
    options:
        knp_menu:
            enable : true
            main_menu: 'main_menu'
            breadcrumb_menu: 'breadcrumb'
hounded commented 6 years ago

just tried to upgrade had to roll back to 2.0.0-beta.5 as @benr77 described

Has this not been fixed or am I missing something @shakaran

regarding

avanzu_admin_theme.theme
shakaran commented 6 years ago

@hounded please upgrade to master or 2.0.0-beta.10, no 2.0.0-beta.5

@ekussberg 1.3.8 doesn't have the format that you describe. Thats for 2.x