Poolers and Models can now be registered as services

[mvrhov]

This is an WIP but I'd like to receive some comments if possible before continuing. With this request the poolers and models can now be registered as as service.

I'd like to make A BC break remove the class:session_builder as it doesn't make much sense anymore.

If there are multiple services one can tag the models or model layers with an attribute named pooler, which is a service id into which the model should be registered.

[mvrhov]

The only problem remaining is that I'd like to inject/use the model via service id e.g

<argument type="service" id="my_bundle.model.my"/>

This right now results in Model class ''K\\Bundle\\FooBundle\\Model\\DefaultDb\\MyModel' is not registered against the session. Which seems logical as it seems that getModel in service('pomm')['default_db'] initializes the model. Using it as a service also means that auto-wire functionality can be used. That doesn't work if expression method must be used e.g.

<argument type="expression">
   service('pomm')['default_db'].getModel('K\\Bundle\\FooBundle\\Model\\DefaultDb\\MyModel')
</argument>

@sanpii, @chanmix51 any hints?

[sanpii]

Behat has a class resolver to do that, but i didn’t find this in symfony.

[mvrhov]

The closest I've gotten is adding the following to the my_bundle.model.my definition

<call method="initialize">
  <argument type="expression">service('pomm')['default_db']</argument>
</call>

so making each pomm db (session) a service would get rid of the expression.

[chanmix51]

Nice work. What kind of problem are you trying to solve ?

[mvrhov]

Primarily this one the 2nd one is having model available as service so it can be injected where it's needed.

[mvrhov]

Can somebody make a review, before I write a couple of lines of documentation?

[chanmix51]

I must say « good job ». I think one should try on an own sf project to ensure there are no BC because of the SessionBuilder change. @sanpii ?

[mvrhov]

@chanmix51: Sometimes I'm getting Model class ''K\\Bundle\\FooBundle\\Model\\DefaultDb\\MyModel' is not registered against the session. Who is supposed to register the Model e.g call initialize on it. I cannot find this in the original ModelManager. If I call it in createClient then I'm getting the error from session that the model is already registered.

[chanmix51]

Oh… this is annoying because it means there are missing unit tests for this section. There can be phantoms clients 😞 Isn’t that a problem of leading/trailing backslash ?

[mvrhov]

I don't think so. I can try to debug it. but I would really need to know who+s supposed to call the initialize on the model.

[chanmix51]

Ah sorry, I misread your previous message. The pooler is responsible of initializing the clients.

[mvrhov]

Hm.. Can you show me the line in ModelManager that does that. All I can find is the definition for the function on the model.. but nothing that call is.

[chanmix51]

This is located in Foundation, the ModelPooler uses the ClientPoolerTrait to implements these functions to register client against the Session.

[mvrhov]

This finally works as expected and is ready for the final review. @stof can you please comment on this.

Now what's tricky here is that for this to work properly the user defined service has to be renamed and a new one with a factory has to be put instead of the old one. Now I find this a bit of magic, but otherwise everyone has to define two services and also define the auto wired types on the correct one for auto wiring to work.

[mvrhov]

IMO, this is ready to merge.

[sanpii]

Just a quick idea (I don't know if it's a good or a bad idea and if it's easy to implement): keep the class:session_builder parameter and allow class or service (starting with a @) names.

[mvrhov]

Having a colon : in a key just got complicated in latest yaml component, The key would need to be quoted "class:session_builder" e.g so I'd leave this deprecated and also deprecate the "pomm:default" in a new PR and make this similar to doctrine e.g 'default_session: true' Regarding the @. this is yaml specific and would mislead the people that when writing configuration in xml, or php, that they are supposed to define the service and not the service name.

The other thing is to be able to support models and layers as a service the session builder in a bundle has to be used. I'd say that most people use default builder and the would not need to change a thing.

[mvrhov]

If I have a custom session builder (an example) I must recreate it via service. I just would like define a new service and use it in the configuration.

The configuration always uses the service id as a parameter never the service "definition". Take a look at framework configuration.

[sanpii]

The configuration always uses the service id as a parameter never the service "definition". Take a look at framework configuration.

Yes, but I would do something like that:

my.pomm.session.template_query:
    class: 'Sanpi\PommProject\TemplateQuery\SessionBuilder'

my_db:
    dsn: 'pgsql://%db_user2%:%db_password2%@%db_host2%:%db_port2%/%db_name2%'
    session_builder: my.pomm.session.template_query

Not redefining my session builder in service container via the pomm.poller tag.

[mvrhov]

It seems that you misread something. You do define the session builder that way. Only poolers and models/layers need to be tagged

[sanpii]

It seems that you misread something.

It’s always a possibility 😃

You do define the session builder that way. Only poolers and models/layers need to be tagged

Yes, but I must copy methods from PommProject\PommBundle\Model\SessionBuilder to prevent errors. It isn’t possible to register pooler directly in the pooler compiler pass? Maybe like in DatabaseCollectorConfigurator.

[mvrhov]

I hope this is what you wanted. Now if you confirm this I'll move ChainConfigurator into the bridge. I think this is more proper place for that class.

[mvrhov]

@sanpii ping :)

[chanmix51]

What's new here ?

[mvrhov]

Waiting for feedback. Also autowire_types is deprecated and they recommend using aliases since 3.3. Another thing is that 3.3 also supports service locator, so injecting whole container is not necessary anymore. However this means, that the 3.3 will be minimum supported version.

[lunika]

This PR is great, I like it :+1:

[sanpii]

Merged, thank you for this great PR 😃

[mvrhov]

Will probably do another PR as soon as sf 3.3 stabilizes. There were a lot of DIC changes that would be nice to have.