Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding "Cookbook" section in documentation in regards to injecting dependencies into generated form types #285

Closed
TheCelavi opened this issue May 27, 2016 · 13 comments
Labels

Comments

@TheCelavi
Copy link
Contributor

By using this bundle on one of the bigger project, we stumbled upon the issue of injecting additional dependencies into auto-generated form types.

We had like 50+ form types which required additional injection of services per requirements, and we needed a neat, fast, productive way to do so.

So, we figure out that we can do that by using traits and compiler pass to analyse service definitions and to inject services where certain traits are used.

This have really busted our productivity, so we decided to create a bundle: https://github.com/RunOpenCode/traitor-bundle and this project was motivation for it.

We believe that it can be useful to provide other users of GeneratorBundle with this info, so I was wandering if you agree for me to add additional cookbook article regarding this bundle that can help users of GeneratorBundle?

Thanks.

@coke54
Copy link
Contributor

coke54 commented May 28, 2016

+1

1 similar comment
@lidija83
Copy link

+1

@bobvandevijver
Copy link
Member

I would say no, as you already state that the bundle is basically breaking Symfony convention. I also find automatic injection of services anywhere kinda shady and very error prone.

Not sure how the others think about it though. @loostro @sescandell.

@TheCelavi
Copy link
Contributor Author

@bobvandevijver If we discuss about TraitorBundle breaking Symfony convention, we can discuss about GeneratorBundle braking Symfony convention too, we are all well aware that each generated form by GeneratorBundle is "magically" registered within service container (Admingenerator\GeneratorBundle\DependencyInjection\AdmingeneratorGeneratorExtension::registerFormsServicesFromGenerator), so in that term, both of stated bundles breaks Symfony convention of registering service: GeneratorBundle does not transparently expose service configuration for modification in separate configuration.

Correct me if I am wrong, of course.

Pros and cons provided in documentation is for people to understand details about implementation of bundle and to make informative decision weather they are going to use it or not.

Almost every known development solution for whatever have its "pros" and it "cons" -> we are deciding about its usage if "pros" outweights its "cons" and if there are no better solutions offered.

In bundle documentation I have explained the issue that we had -> app. 50+ generated forms, all of those forms required additional service injection.

It was ridiculous unproductive for us to go trough 50+ service definitions, one by one, and adding definitions about additional injection.

Our solution was to build something elegant which will increase our productivity, and, according to our estimation, we have saved at least 8 working hours for this project.

For us, breaking Symfony convention by using this bundle in order to gain productivity was excellent trade off, and we are going to use it for all our future project based on GeneratorBundle (at least until better solution is offered).

This is our solution, in text above is our positive experience, we are very satisfied with it and we are offering our solution for free, because we believe that others with similar context of problem (several dozens of auto-generated forms) can benefit with this approach, at the cost of a little sacrifice of convention.

@bobvandevijver
Copy link
Member

No, registering services in the kernel during compile passes is not magical and is actually used in a lot of 'normal' processes in the Symfony system, for example for authentication. That being said, I just inspected the code of your bundle a bit more and it appears have less impact than your readme depicts. I was thinking about rewriting the classes and services definitions to add the required services, but it is simply done with setters (which might be more memory consuming actually, not sure about that).

So, feel free to create the PR with the cookbook entry. That way we can give also feedback on the actual piece. It should of course contain all information to make an educated choice of using the bundle.

However, I must ask, why do your need so many services in your forms? I guess I have 200+ forms, and only a few of them really require services for functioning.

@TheCelavi
Copy link
Contributor Author

No problem - it is a still project in progress, will be released in couple of weeks. It is financial type of software, dealing with bills and invoicing and those kind of very interesting stuff....

In general, our forms required additional service dealing with, per example, exchange rates, taxes, ledger, as well as various configuration services...

Majority of forms required injection of one additional service, some of the forms required several services.

Our first 'goto' solution was writing compiler pass, getting a service form definition, modifying either constructor or adding method call definition.

We soon realised that we are going to need some kind of elegant solution of injection, and since forms particularly does not need constructor injection in most of the time, we asked our self, how cool would be to just say:

class NewForm extends AbstractType
{
    use \Louis\InterfaceBundle\Form\Type\Traits\ExchangeRateTrait;
}

and magically, our service would get injected in our form?

Now, this is a witchcraft, A F*** WITCHCRAFT! :) but it just saves time, especially when actively developing, and client is constantly adding/removing stuffs... It was not just providing service once for few forms, it was removing, adding, changing what form has to do...

@ioleo
Copy link
Member

ioleo commented May 31, 2016

I see no harm in adding a cookbook. The bundle itself provides several warnings/notices in readme/docs, so it anyone misuses it, well... :P

@sescandell
Copy link
Member

Hi there,

Just wondering, not taking into account your situation where you have many forms to update: don't just redefining the same service in the service file would be enough?

Example:

# app/config/config.yml file
...
services:
    admingen_generator_mymodel_edit:
        class: FQDN\To\My\Generated\EditForm
        arguments: [@my_custom_service]
        calls: [{setAuthorizationChecker, [@security_checker]}]
        tags: 
            - { name: form.type }

If this is working, I think it should be the information provided into the documentation

@TheCelavi
Copy link
Contributor Author

@sescandell I will check just to be sure, but, as far as I am familiar, service definition is first loaded from configuration file, then compiler pass takes over, and then, I believe, service defined within configuration gets overridden.

However, I will check just to be sure.

@sescandell
Copy link
Member

@sescandell I will check just to be sure, but, as far as I am familiar, service definition is first loaded from configuration file, then compiler pass takes over, and then, I believe, service defined within configuration gets overridden.

Agree with you, except that registerFormsServicesFromGenerator is called on Bundle configuration which is not a Compiler Pass and normally loaded before the app/config/config.yml file

Waiting for results of your tests.

Thank you !

@TheCelavi
Copy link
Contributor Author

@sescandell Yes, it is an in extension, and extension is fired up before any compiler pass, but after configuration is loaded as well, which kinda makes sense if you think about it...

I just tested to confirm - and answer is no, you can not redefine service in configuration file, it must be in either extension which fires up after GeneratorBundle extension (not reliable) or in compiler pass (reliable).

TheCelavi added a commit to RunOpenCode/GeneratorBundle that referenced this issue Jun 1, 2016
@sescandell
Copy link
Member

sescandell commented Jun 1, 2016

Yes you're right!

Thanks 👍

@TheCelavi
Copy link
Contributor Author

This is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants