WIP: Default injected services #3319

greg0ire · 2015-10-03T13:59:19Z

writing the docs
writing the tests
writing the code

This is an RFC Some questions before I start :

What do you think about the configuration I documented ? I'm not sure it will be sufficient, because I think the strategy should only be applied on admins defined by the user. Maybe there should also be a configuration key whitelisting bundles that should be subject to this strategy ? @OskarStark @soullivaneuh please advise.
What should happen when the users customizes the global label translator strategy, and specifies another strategy on a given admin ?
I'm basing this P.R. on the 2.3 branch, I think it can be fully B.C.

rande · 2015-10-03T14:04:38Z

Did you see this documentation : https://sonata-project.org/bundles/admin/master/doc/reference/translation.html#label-strategies ?

greg0ire · 2015-10-03T14:05:55Z

Did you see this documentation : https://sonata-project.org/bundles/admin/master/doc/reference/translation.html#label-strategies ?

Oh my god no… how did I miss that ?

rande · 2015-10-03T14:07:29Z

I should add a search engine :)

greg0ire · 2015-10-03T14:09:15Z

I should add a search engine :)

Yeah, and I should get some sleep :P

greg0ire · 2015-10-03T14:10:23Z

I'm going to merge both docs.

After reading the compiler pass that applies the default translator strategy, I'm under the impression that the default actually is the native translator strategy.

It is the use case for definition lists.

Closes sonata-project#2419 When people configure this, they probably want to do it on a global basis rather than for each of their admins.

OskarStark · 2015-10-08T14:02:03Z

any news @greg0ire

greg0ire · 2015-10-08T14:03:33Z

I'm waiting for your input on item 1.

soullivaneuh · 2015-10-08T14:05:56Z

First of all, are we talking about a new feature or a fix?

OskarStark · 2015-10-08T14:09:14Z

using an existing feature more easily by a global config option, right?

greg0ire · 2015-10-08T14:09:40Z

It's a new feature : being able to configure strategies globally. For the moment, you have to set them admin by admin, which is a bit painful.

I think the best would be if every bundle could have its own strategy :

sonata_admin:
   label_translator_strategy:
      bundle1: some_strategy
      bundle2: some_other_strategy

greg0ire · 2015-10-08T14:10:10Z

using an existing feature more easily by a global config option, right?

exactly, that would avoid a lot of repetition if one's admin.yml

OskarStark · 2015-10-08T14:12:30Z

I think the best would be if every bundle could have its own strategy :

what about:

sonata_admin:
   label_translator_strategy:
    global: some_strategy
    bundles:
        bundle1: some_strategy
        bundle2: some_other_strategy

soullivaneuh · 2015-10-08T14:15:11Z

As new feature, it should be on master and not 2.3.

greg0ire · 2015-10-08T14:16:07Z

As new feature, it should be on master and not 2.3.

Duly noted, I'll fix that

greg0ire · 2015-10-08T14:16:50Z

@OskarStark : not sure about global : it could apply to vendor bundles that did not specify anything, and break their translations

greg0ire · 2015-10-08T14:18:13Z

Maybe this would be better :

sonata_admin:
   label_translator_strategy:
       bundles:
           some_strategy: [bundle1, bundle2, bundle3]
           some_other_strategy: [bundle4, bundle5]

greg0ire · 2015-10-08T14:19:13Z

@OskarStark : not sure about global : it could apply to vendor bundles that did not specify anything, and break their translations

Although global should be optional, so if it breaks anything, "just do not use it" seems sensible

greg0ire · 2015-10-08T14:22:04Z

Other question, what should bundle1 exactly be ? A namespace prefix ? In which case someone could actually match several bundles with one prefix, like MyCompany ?

OskarStark · 2015-10-08T14:57:23Z

not sure about global : it could apply to vendor bundles that did not specify anything, and break their translations

you're right, global is not a great idea...

like this version of you, it looks wrong with bundles:

sonata_admin:
   label_translator_strategy:
       bundles:
           some_strategy: [bundle1, bundle2, bundle3]
           some_other_strategy: [bundle4, bundle5]

i think you mean something like this?

sonata_admin:
   label_translator_strategy:
       strategies:
           some_strategy: [bundle1, bundle2, bundle3]
           some_other_strategy: [bundle4, bundle5]

greg0ire · 2015-10-08T15:00:05Z

Looks better indeed! What about the "bundle1" reference ? What would you actually use here ?

rande · 2015-10-08T15:08:17Z

I don't see the use case where this should be useful. It will be a pain to update the provided the translation for the new generated keys.

OskarStark · 2015-10-08T15:10:30Z

What would you actually use here ?

sonata_admin:
    label_translator_strategy:
        strategies:
            sonata.admin.label.strategy.native:
                - app.admin.foo
                - app.admin.bar
            sonata.admin.label.strategy.underscore:
                - app.admin.product

i would recommend to use the admin service id, because the strategies are already applied on the admin service configuration, what do you think?

greg0ire · 2015-10-08T15:13:53Z

I don't see the use case where this should be useful. It will be a pain to update the provided the translation for the new generated keys.

It would not be configured globally by the bundle user. Instead, each bundle would merge their sonata_admin configuration with the user's configuration. Which means it should be mergeable. Which means we cannot really use the strategies key, @OskarStark, but stick with the bundles key.

greg0ire · 2015-10-08T15:16:01Z

i would recommend to use the admin service id, because the strategies are already applied on the admin service configuration, what do you think?

I was thinking about something more general, like a bundle name or a namespace prefix, so that you do not have to specify every admin in your bundle (it is unlikely that you would mix strategies in a given bundle, but you should still be able to by not using this new feature and sticking with what already exists).

rande · 2015-10-08T15:16:52Z

The feature already exists: https://github.com/sonata-project/SonataAdminBundle/blob/master/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php#L225-L248

And it is documented: https://sonata-project.org/bundles/admin/master/doc/reference/advanced_configuration.html#service-configuration

:)

greg0ire · 2015-10-08T15:18:46Z

@rande :

When you create a new Admin service you can configure its dependencies, the services which are injected by default are:

You cannot change the defaults for all the admin in your bundle, can you ? You are forced to configure it for each and every admin, aren't you ?

greg0ire · 2015-10-08T15:21:48Z

I think in the end it should look like that :

# config.yml
sonata_admin:
    label_translator_strategy:
        bundles:
            MyBundle: my_strategy_id

Then, in sonataUserBundle, you would prepend this config:

sonata_admin:
    label_translator_strategy:
        bundles:
            SonataCoreBundle: whatever.strategy.is.used.in.the.core.bundle

rande · 2015-10-08T15:23:13Z

@greg0ire no, and I don't see the use case where you need to update all services. It is very sensible, and I guess it will be easier for someone you really want to do that to create a dedicated CompilerPass.

rande · 2015-10-08T15:25:24Z

if this should be implemented, it will be better to have:

sonata_admin:
     default_services:
           model_manager: the service id
           form_contractor: another service id
           label_translator_strategy: sonata.admin.label.strategy.native

greg0ire · 2015-10-08T15:27:17Z

@rande : here is how my admin.yml file looks at the moment :

    admin.library:
        class: SomeAdmin
        tags:
            -
                name: sonata.admin
                manager_type: orm
                label_translator_strategy: "sonata.admin.label.strategy.underscore"
    admin.admin_entity:
        class: SomeOtherADmin
        tags:
            -
                name: sonata.admin
                manager_type: orm
                label_translator_strategy: "sonata.admin.label.strategy.underscore"
    admin.prospect:
        class: YetAnotherAdmin
        tags:
            -
                name: sonata.admin
                manager_type: orm
                label_translator_strategy: "sonata.admin.label.strategy.underscore"

How can I avoid repeating the label_translator_strategy ? I'm using that on
every project, because I think it is far better that the native strategy, I'm
never going to use anything else in my bundles. Am I missing something ?

greg0ire · 2015-10-08T15:30:39Z

if this should be implemented, it will be better to have…

Indeed, let's be more generic. But I think there should be an additional thing to specify : what should this defaults be limited to ? For instance, people probably do not want to change the strategy for, say, SonataUserBundle… so it should probably be :

sonata_admin:
    default_services:
        my_bundle:
            model_manager: the service id
            form_contractor: another service id
            label_translator_strategy: sonata.admin.label.strategy.native

That way, SonataUserBundle can also specify its config and get it merged with
the final user's config.

rande · 2015-10-08T15:42:07Z

@greg0ire you are also repeating the name, and the manager_type. IMHO, it is better to be explicit on what you want. There is no performance gain with your proposal. It just another vector of configuration complexity.

greg0ire · 2015-10-08T15:46:10Z

you are also repeating the name, and the manager_type

I wouldn't if I could…

There is no performance gain with your proposal.

That's not what I was looking for, I was looking for better DX

It just another vector of configuration complexity.

I get that, there are already many way to change the strategy, but it is not possible globally… too bad. But in the end it's your call, so I am closing.

rande · 2015-10-08T15:49:46Z

@greg0ire The feature can be implemented in a dedicated bundle, also I am not the only one to choose. I just raise my voice ;)

greg0ire · 2015-10-08T15:51:54Z

Ok, @OskarStark @soullivaneuh , what do you think ? Useful or not ? Do not hesitate to say if not, I haven't coded anything yet, so, no worries :)

OskarStark · 2015-10-08T15:57:54Z

what about using the proposition by @rande

sonata_admin:
     default_services:
           model_manager: the service id
           form_contractor: another service id
           label_translator_strategy: sonata.admin.label.strategy.native

but IF the strategy is set directly on an admin, this one gets used?

same like the group for example:

            sonata.admin.group.settings:
                label:           navigation.folder.settings
                label_catalogue: messages
                icon:            '<i class="fa fa-cog"></i>'
                items:
                    - setting.admin.portal_setting

services:
    setting.admin.portal_setting:
        class: SettingBundle\Admin\PortalSettingAdmin
        arguments: [~, SettingBundle\Entity\PortalSetting, SettingBundle:CRUD]
        tags:
            - {name: sonata.admin, manager_type: orm, group: admin, label: Portal Setting List }

In this case group:admin is specified, but "overwritten" by the navigation configuration.

For the strategies i would like to use the reverse case. this already helps us to stay BC, because if its set directly on an admin, it works again

greg0ire · 2015-10-08T16:07:14Z

but IF the strategy is set directly on an admin, this one gets used?

My point exactly, we are going to replace the defaults in the compiler pass

greg0ire force-pushed the default_label_translator_strategy branch from 9d63a2a to 831b7d7 Compare October 3, 2015 14:21

greg0ire added 6 commits October 3, 2015 18:40

synchronize comment with the current behavior

4133fe0

After reading the compiler pass that applies the default translator strategy, I'm under the impression that the default actually is the native translator strategy.

describe the underlying interface

92f24a0

use definition lists

b08ab03

It is the use case for definition lists.

document bc label translator strategy

788b927

add call for contributions

0810a52

expose the global label translator strategy

b4e7482

Closes sonata-project#2419 When people configure this, they probably want to do it on a global basis rather than for each of their admins.

greg0ire force-pushed the default_label_translator_strategy branch from 831b7d7 to b4e7482 Compare October 3, 2015 16:40

greg0ire changed the title ~~WIP: Default label translator strategy~~ WIP: Default injected services Oct 8, 2015

greg0ire closed this Oct 8, 2015

This was referenced Dec 17, 2015

can not activate sonata.admin.label.strategy.underscore #3190

Closed

[Feature request] Default label translator strategy #2419

Closed

greg0ire mentioned this pull request Feb 8, 2016

set alias default translation strategy #3572

Closed

greg0ire mentioned this pull request Sep 10, 2017

allow to change 'label_translator_strategy' for all admins at global config of 'sonata_admin.admin_services' key #4637

Closed

greg0ire deleted the default_label_translator_strategy branch January 7, 2018 12:47

WIP: Default injected services #3319

WIP: Default injected services #3319

Conversation

greg0ire commented Oct 3, 2015

rande commented Oct 3, 2015

greg0ire commented Oct 3, 2015

rande commented Oct 3, 2015

greg0ire commented Oct 3, 2015

greg0ire commented Oct 3, 2015

OskarStark commented Oct 8, 2015

greg0ire commented Oct 8, 2015

soullivaneuh commented Oct 8, 2015

OskarStark commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

OskarStark commented Oct 8, 2015

soullivaneuh commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

OskarStark commented Oct 8, 2015

greg0ire commented Oct 8, 2015

rande commented Oct 8, 2015

OskarStark commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

rande commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

rande commented Oct 8, 2015

rande commented Oct 8, 2015

greg0ire commented Oct 8, 2015

greg0ire commented Oct 8, 2015

rande commented Oct 8, 2015

greg0ire commented Oct 8, 2015

rande commented Oct 8, 2015

greg0ire commented Oct 8, 2015

OskarStark commented Oct 8, 2015

greg0ire commented Oct 8, 2015