search

symfony-cmf / seo-bundle

A SEO Solution for duplicate contents, page titles, etc.
https://cmf.symfony.com
47 stars 27 forks source link

more specific DI configuration definition #25

Closed dbu closed 10 years ago

dbu commented 10 years ago

the Configuration should use enumNode where we have a limited set of choices, rather than scalarNode. see for example https://github.com/symfony-cmf/MenuBundle/blob/master/DependencyInjection/Configuration.php#L41 (and btw, we need this use_sonata_admin option for the sonata admin extension too)

ElectricMaxxx commented 10 years ago

Ahh ok this would make sense for both strategies, but for the separator too?

The use_sonata flag should enable/disable the AdminClass, right? But how can i disable sonatas AdminClass? When it's created in the admin.xml it is shown on dashboard. I need to have a look in the other bundles.

ElectricMaxxx commented 10 years ago

changed it to the enumNode with the options. i in master now.

ElectricMaxxx commented 10 years ago

Implemented the function of loading the admin.xml or not as it is done in the menu-bundle depending on the setting of use_sonata_admin. Seems to be read, but need a chance to test it, but got issues on testing (https://github.com/ElectricMaxxx/CmfSeoBundle/issues/10). Need to get the tests first before closing this one

ElectricMaxxx commented 10 years ago

done for the moment.

dbu commented 10 years ago

exactly, the separator is just a string, there should be no restrictions on it. about phpcr persistence, we can make CoreBundle aware of the seo bundle once its integrated, and have it prepend the right configuration.

ElectricMaxxx commented 10 years ago

@dbu you mean i shouldn't have added this:

https://github.com/ElectricMaxxx/CmfSeoBundle/blob/master/DependencyInjection/Configuration.php#L25

cause i tried to get the same structure as the other bundles have. Added the function for being aware of use_sonata_admin in Extension, too

dbu commented 10 years ago

ah, sorry. no that seems absolutely right.

what i meant is just that usually the user should not need to configure it here, but globally on cmf_core. created #34 about this - maybe you could do the PR already now, so that we can have it in the cmf 1.1