symfony-cmf / menu-bundle

Extends the KnpMenuBundle to work with PHPCR ODM
https://cmf.symfony.com
32 stars 48 forks source link

undeclared dependency on sonata admin #5

Closed petesiss closed 12 years ago

petesiss commented 12 years ago

If there is to be a dependency on sonata admin by default, then it should be stated in composer.

https://github.com/symfony-cmf/MenuBundle/blob/master/DependencyInjection/Configuration.php#L25

But I think actually it should default to false and the Sandbox should specifically set it to true if it is using it. Though actually now I think of it, is it right even to have any specific mention of a specific admin system? These bundles should be completely free of any ties to specific admins shouldnt they?

uwej711 commented 12 years ago

The idea is to have it there as a default implementation but checking the configuration whether the application has sonata admin or not (this still needs to be implemented). Additionally there will be a flag to suppress registration of this admin class even if sonata admin is present.

So the bundle should not depend on sonata-admin but the sandbox should.

petesiss commented 12 years ago

This should default to false then, with a "true" entry in the sandbox config.yml I think.

Otherwise when you try to use the menuBundle in a project you get an immediate bug where it can't find the admin service.

uwej711 commented 12 years ago

To work around that issue now set

symfony_cmf_menu:
    use_sonata_admin: false

in your config.yml

dbu commented 12 years ago

i propose that we default to "auto" which makes the configuration thing check if the class exists. if i set it to true explicitly, i need some exception telling me i borked my install, not silently not work.

btw, the option should probably be use_sonata_phpcr_admin in case somebody ever writes model mappings for orm and an admin for orm.

petesiss commented 12 years ago

@uwej711 yes, of course, but I think the point is it probably shouldn't require that config to avoid the error, even if it is a wip.

petesiss commented 12 years ago

"auto" sounds like a good idea. As long as you dont get an unexplained error when trying to use the bundle without sonata admin.

Longer term, even mentioning a specific admin anywhere other than the sandbox seems like it should be avoided if at all possible. Making concessions for a specific admin in core code seems like it undermines the idea of the cmf.

uwej711 commented 12 years ago

@dbu and I discussed that and came to the conclusion that if we want to provide an admin going that way would be rather sensible. The alternatives are

  1. no admin at all
  2. create one bundle for all document admins
  3. create a separate bundle for admins for each bundle
  4. forces everybody to some copy and paste
  5. makes this bundle dependent on all cmf bundles
  6. seemed like a bit overkill (it's basically just one class we are talking about)

So it came to adding that to the bundle. I'm sorry I was too blind to think of cases where sonata admin in not used. But there we have a general discussion on what bundles the cmf should depend and on which not (think of FOS, Knp etc.).

I will try to get the auto mode done quickly.

petesiss commented 12 years ago

Ok. I dont know enough about these bundles to understand the need for the admin specific stuff, so happy to concede on that if the general consensus is that it makes sense.