[WIP] common collaborators

[docteurklein]

I'm currently implementing a Maintainer for ContainerAware objects that creates preconfigured collaborators.

For example, in a spec:

<?php

use PhpSpec\ObjectBehavior;

class TestControllerSpec extends ObjectBehavior
{
    public function it_generates_url($container, $router)
    {
        $this->setContainer($container);
        $this->generateUrl('homepage')->shouldReturn('test');
    }
}

As you can see, the container is preconfigured to return a RouterInterface instance for $this->container->get('router') call.
Same for common services like request, session, ...

This would superseed the ContainerInjecterMaintainer + the ContainerInitailizerMaintainer.

I also consider the method I describe a bit cleaner, for the following points:

pros

• no more ControllerBehavior extends ObjectBehavior
• no $this->container property in the spec. (if we start to do that, we'll soon have dozen of spec properties)
• no extended Container
• common services classes and names are configurable via DIC / phpspec.yml
• you can override them per spec method (docblock or typehint)

cons (if any):

• you have to follow naming conventions to access common services
• you separate / move complexity in phpspec.yml, which makes the spec class not anymore self-contained.

Adding your own common collaborators

There is already an entry point to add your own, which is to add a key into the symfony2.common-collaborators param.

# phpspec.yml
extensions: 
    - PhpSpec\SymfonyExtension\Extension

symfony2_extension.common-collaborators:
    verySpecialCollaborator: A\Very\Special\FQNS

This array supports 3 different syntaxes:

• full: name: {containerIdName: FQCN }
• simple: name: FQCN
• null: name: ~

In the last case (null), the className configuration is delegated to initializers (f.e: request ).

You can also register an initializer that will add more behavior to $verySpecialCollaborator.

Just implement the supports($name) method (and others) and register a service with prefix collaborator.initializer.

PS: the maintainer will apply only if CUS (class under spec) implements ContainerAwareInterface. That's maybe a problem.

I'd be curious to have your opinions on this @everzet @jakzal @MarcelloDuarte .

[docteurklein]

for the example. (to be removed)

[jakzal]

I was actually planning to register common service by default. I also don't like the fact we need to call $this->container->set() atm.

However, I wouldn't limit the registration of collaborators to the common ones. It would be nice to have them by default, but it would also be nice to be able to add more of them in an easy way (without the need of putting them to the container "manually"). It would also make it easier to implement automatic registration of new services in a DIC config file.

As for the extended container - I think it's no longer needed (there were issues with earler phpspec versions, i have to check that again).

One proposition: I found it really useful to describe features in Gherkin (initially in the issue description on github). Can we make an experiment and try working on new stuff in this way? It's easier to understand what needs to be implemented and more people could pick up the tasks.

I won't be able to give it some more thought before Symfony Live :(

[docteurklein]

yeah sure! I wanted to create scenarios with this.
Shame on me, I didn't started with this :)

I will write some gherkin to describe the poposed behavior.

[docteurklein]

Would you like me to continue on this feature?

Also, concerning your point about a config file that describes what are common collaborators (right?),
there is already a DIC parameter that could do that.

It's just empty atm.

[cordoval]

is this using the new options parameters PR from phpspec/phpspec?

[docteurklein]

I reverted it until (if) the PR is (not) accepted.

[docteurklein]

it was breaking travis, cause composer was not using the PR'd version.

[gquemener]

🐴

[docteurklein]

what with that ? 🐫

[cordoval]

👶 maybe he means the @ handling

[docteurklein]

yeah :) it's acceptable in that case imho

[cordoval]

si un médecin le dit, nous buvons 👶

[docteurklein]

haha :) don't trust me.

[docteurklein]

ping @jakzal

[jakzal]

@docteurklein I'm looking into it and soon will express my opinion :)

We have to be really careful on what we decide now. PhpSpec encourages to use good coding practices and makes writing bad code painful. We should follow the same values :)

[jakzal]

First of all, great work @docteurklein! I'm sorry I didn't come back to you earlier.

I'm a bit concerned that with this PR we're making too many things implicit. Reading a spec no longer tells us the whole story, since the way collaborators are created would be hidden.

The value of specing controllers is that we're being forced to make them simple. As soon as we get to mock too many collaborators, or handle too many execution paths, we're starting to think whether there's a concept missing. That's when we start thinking of yet another collaborator which could hide interactions with the others (for example).

Implicit mocking would encourage bad practice I'm afraid. At least it wouldn't discourage it.

I think It should be at least visible in the spec with what kind of services we intend to work with. I'm definitely voting 👎 against defining collaborators in the config file (as the config file is centralised). I know I should proposed another way, but I didn't think this part through yet.

As for the use of $this->container I'm not gonna be stubborn. If the community wants it out, lets remove it ;) I thought it's a good idea since every method in a spec class is actually called on the SUS. At a time, I thought it makes sense to access the container in a similar way. What I didn't think of is I'm tying it to the specific container implementation which defines the container property.

We could replace the whole container injector thingy with a generated spec, which would contain code we intend to write most of the time:

class UserControllerSpec extends ObjectBehavior
{
    function let(ContainerInterface $container)
    {
        $this->setContainer($container);
    }

    function it_is_container_aware()
    {
        $this->shouldHaveType('Symfony\Component\DependencyInjection\ContainerAwareInterface');
    }
}

I count on a good discussion before we decide. The decisions we'll make now are very important as they'd be hard to roll back.

[docteurklein]

Hey! Very happy to read your opinion :)

I totally agree on the fact we shouldn't facilitate bad practices.
One of them is being ContainerAware.

I also agree on the problem of separating some parts in the yaml config and the other in the spec class, making the latter not anymore self-contained.

I didn't really think of these 2 points, at the time I wrote this :/

My goal was to avoid long boilerplate code for controller specs.
This could actually be done at the language level, using traits, or inheritance, or some reusability pattern, but:
- We should avoid inheritance usage
- Traits (could -think let override-) still need some boilerplate to take advantage of it (and 5.3 :/ )

But as you said, if we start to write loooong let methods, we're doing it wrong, and we should feel some pain,
not hide it in a config file.

Still, I see some advantages of predefinied mocks. For example, doctrine.
Doctrine Registry::getManager (always) returns an ObjectManager, which itself, by default, returns a ObjectRepository. This common logic could be shared across spec classes.

[docteurklein]

Same thing for Request object.
Its attributes property should always points to a ParameterBag instance.
There is no advantage of letting the spec class configure something it doesn't own (the request object).
Having to mock everytime the ParameterBag's request is sad, because it's a constant.

So, do you agree on the fact we should have preconfigured mocks at least ? (whatever the method.)

[cakper]

I'd like to add my thoughts on this PR :)

First thing - totally agree that mocking Request every single time is painful and having it preconfigured is a good thing 👍

According to discussion that we had with @richardmiller and @jakzal I think we should avoid injecting instance of container to our controllers and instead inject only services that are needed and define controller as a service itself.

Because injecting container was introduced to ease Symfony learning curve I think it is useful on the beginning, but then people are bit stuck with this idea. My point is to encourage people to make dependencies explicit and force them to create thin controllers which leads to create services that do only stuff we need in this particular case and inject them to the controller. I understand that some peopled don's see that much value in this approach but I think somehow forcing it would be beneficial.

The other thing is there is no one "right way" of writing controllers ;) And closing this extension to only one solution is not the best thing. As @everzet said we should think about some extension points to allow people make use of their workflow (if they have it :) ).

[peterjmit]

I don't think this extension should set out to facilitate a practice which is ostensibly poor, and potentially in a way that makes the tests a little less obvious.

Anecdotally having just refactored a non-trivial codebase to use phpspec/prophecy, if the container had been used a lot less as a service locator (mainly in controllers) my job would have been a lot easier.

I think this extension should facilitate working with Symfony where the user has less of an option, for example as has already been mentioned mocking/stubbing the Request object can be very verbose and help with reducing that would make tests involving it a lot cleaner.

[awildeep]

I have to agree with @peterjmit.

It seems to me the issue is in the difficulty in making controllers as services (it has its own dedicated web page; http://symfony.com/doc/current/cookbook/controller/service.html), not in testing controllers that are already controllers that have properly defined dependancies.

Perhaps a specific set of Symfony2 description generators are a better location to target for this? Something that would simplify the workflow of just making a controller as a service. How about:

bin/phpspec sf:service AcmeBundle/MyController EngineInterface:@templating

bin/phpspec sf:service {ControllerAsServiceName} [{Datatype:Dependency} [{Datatype:Dependency} [...]]]

This could build a controller named MyController inside of AcmeBundle, and generate the boilerplate (yml/xml/etc) to pass in the template dependency as well as allow the typecast to be set. (After typing that out, I realize I don't exactly like my syntax structure of my proposed command, but I don't have a more elegant concept yet).

Also, I am linking @peterjmit's blog here, as this has a much better feel for how to accomplish the goal to me.
http://peterjmit.com/blog/refactoring-a-symfony-2-controller-with-phpspec.html

[docteurklein]

Just to be clear: this PR doesn't encourage bad practices, it only preconfigures a mock if you use it.
So if you don't use $container for example, but only use a mock named $request, the $request will be preconfigured.
That's all.

@awildeep concerning generators:

I don't think it's the role of phpspec to build all this (yml, ...). To generate the class/methods is enough.
Currently the phpspec/Symfony2Extension generates a ContainerAware class, if the class is suffixed with Controller.

This PR is more generalistic, as it focuses on every class (currently only ContainerAware classes, but this is an error), not only controllers.

I think we should discuss about how useful it is to have preconfigured mocks.
If it is useful, then this PR is a potential solution.

[jakzal]

@docteurklein Request is an extreme example where having preconfigured stubs would be very useful. It might even be worth considering if this feature should be part of the core.

Anyways, I think the problem of this particular request is that it also relates to the container. My proposition is to take care of these two problems separately.

[stof]

@awildeep The issue most people have when they start using a service as controller is that they don't update their routing to reference the service id instead of the class name, and so the controller resolver does not use their service (it will not magically guess that you have a service defined with this class and it should use it instead of the normal instantiation logic). And the routing for controllers as services is documented. But I suspect people will read only half of the doc only before trying

[awildeep]

@stof It seems to me that is just a matter of making doc more readily available, and perhaps making the tooling easier to use. After all people will choose the path of least resistance most of the time until they have a specific need to do otherwise (I have fallen into this category before for sure). But as I have a goal of easily testable code, Symfony2 Controllers (non-service based) really fight against me.

I am not sure if this particular discussion belongs here; perhaps it belongs as a Symfony2 discussion as a whole.

[docteurklein]

oops Templating

[docteurklein]

I have in mind to decouple "common collaborators" management from symfony2 extension.
It is indeed usable in virtually any other context than symfony2.
Would you mind if I move this feature to an independant extension ?
It would then be easy to use this extension in symfony2 extension to provide sf2 specific initilaizers.