Implemented method calling via __calls #666

alekitto · 2017-01-29T20:57:53Z

Added a SimpleCaller class to handle method calls via __calls as documented here: https://github.com/nelmio/alice/blob/master/doc/complete-reference.md#calling-methods

theofidry

Besides my comment, a few missing changes:

Update the service definition for the Symfony bridge as well
For the local tests you should have a bin/tests.sh script you can run (which runs the tests for the Symfony bridge as well)
You can remove the TODO from the doc
I would like to see an integration test as well (in LoaderIntegrationTest), which ensures it works with both the NativeLoader and the loader registered in Symfony as well

theofidry · 2017-01-29T22:37:14Z

fixtures/Entity/Caller/Dummy.php

+    /**
+     * @param string $title
+     */
+    public function setTitle($title)


you can add the typehint in favour of the phpdoc here

theofidry · 2017-01-29T22:38:50Z

fixtures/Entity/Caller/Dummy.php

+class Dummy
+{
+    private $title;
+    private $foo = 0;


$counter? Even if we don't know the counter of what we would at least get that it's a number

theofidry · 2017-01-29T22:41:58Z

fixtures/Generator/Caller/FakeCaller.php

@@ -23,9 +24,9 @@ class FakeCaller implements CallerInterface
    use NotCallableTrait;

    /**
-     * @inheritdoc
+     * {@inheritdoc}


remove the brackets, they are here for when there is text:

interface IFoo { /** * Something */ function bar(); } class Foo implements IFoo { /** * @inherit // equivalent to "pick all the stuff of the parent method" */ function bar(); /** * {@inherit} // equivalent to "replace this bit by the stuff of the parent method" * * My comments/phpdoc */ function bar(); } /** * My comments //ignore the phpdoc of the parent method */ function bar();

I've taken that from a draft of the PSR-5, it's still a draft so may change, but as the whole project follows the convention I would prefer to keep it (changing it doesn't make much sense at this point IMO)

theofidry · 2017-01-29T22:42:51Z

src/Definition/MethodCallBag.php

@@ -50,4 +50,20 @@ public function isEmpty(): bool
    {
        return [] === $this->methodCalls;
    }
+
+    /**
+     * {@inheritdoc}


same as above

theofidry · 2017-01-29T22:42:52Z

src/Definition/MethodCallBag.php

+    }
+
+    /**
+     * {@inheritdoc}


same as above

theofidry · 2017-01-29T22:51:05Z

src/Generator/Caller/SimpleCaller.php

+
+        foreach ($calls as $methodCall) {
+            $methodCall = $this->processArguments($methodCall, $fixture, $fixtureSet, $scope, $context);
+            $scope[$methodCall->getMethod()] = $methodCall;


I don't think we need to add the method name to the scope. When we inject a property value, we do so because we may need the evaluated value later:

Dummy: dummy: foo: '<evaluate_foo()>' bar: '$foo' # value of '<evaluate_foo()>' (which may differ from '@self->foo')*

*: @self->foo will try to access to the foo property of the object, which might have a different value if the value has been set via a setter which doesn't just do a $this>foo = $foo.

What you are doing is enabling:

Dummy: dummy: __calls: - setFoo: [ '<evaluate_foo()>' ] - setFoo: [ '$setFoo' ]

which I don't think is very useful and even error prone as as you can see in the case above, you may override the value if doing successive calls

theofidry · 2017-01-29T23:55:42Z

tests/Generator/Caller/SimpleCallerTest.php

+        $dummyProphecy = $this->prophesize(Dummy::class);
+        $dummyProphecy->setTitle('foo_title')->shouldBeCalled();
+        $dummyProphecy->addFoo()->shouldBeCalledTimes(2);
+        $dummy = $dummyProphecy->reveal();


to be consistent with the existing codebase, I would add a /** @var Dummy $dummy*/ inline phpdoc between L73 and L74

theofidry · 2017-01-29T23:58:59Z

tests/Generator/Caller/SimpleCallerTest.php

+    {
+        $dummyProphecy = $this->prophesize(Dummy::class);
+        $dummyProphecy->setTitle('foo_title')->shouldBeCalled();
+        $dummyProphecy->addFoo()->shouldBeCalledTimes(2);


I would put shouldBeCalled() here as when you are creating the prophecy, you don't care of how much calls are being made at this point, it's a check you are doing in the end. So at the end of the test you should add:

$dummyProphecy->setTitle(Argument:any())->shouldHaveCalledTimes(1); $dummyProphecy->addFoo()->shouldBeCalledTimes(2);

granted that in this case, this might seem a bit redundant, but it's a practice that usually makes it easier to follow IMO, and again let's try to stay consistent with the codebase

theofidry · 2017-01-30T00:00:19Z

tests/Generator/Caller/SimpleCallerTest.php

+                        null,
+                        new PropertyBag(),
+                        (new MethodCallBag())
+                            ->with(new SimpleMethodCall('setTitle', [$titleValue = new FakeValue()]))


I would add a second call on setTitle to make sure the second overrides the first

theofidry · 2017-01-30T00:02:16Z

src/Generator/Caller/SimpleCaller.php

+            }
+        }
+
+        return $methodCall->withArguments($arguments);


you should return [$methodCall->withArguments($arguments), $fixtureSet]. The reason $result contains both the a value and the set is that the set may have changed during the resolution. A simple example:

Dummy: dummy2: __construct: - '@dummy1' # while resolving the value, the fixture `@dummy1` will be generated dummy1: foo: 'foo'

And if the $fixtureSet is not properly updated, when it will be the turn of dummy1, instead of finding the object already generated, it will generated again (which will also break the dependency between dummy2 and dummy1 as dymmy2#dummy1 will no longer be dummy1)

$fixtureSet here is passed by-ref, so it is already updated.
Do you prefer returning an array with the new fixtureSet instead of passing it by reference?

Ah I missed that, yes please, I prefer explicit returned values over passing them by references :)

theofidry · 2017-01-30T00:08:05Z

Thanks for the PR @alekitto, that's some big work :) There is a few changes still to do, but the hardest has been made.

How was the dive in the code? I tried to make it readable but if you have some feedbacks and things that looked really weird don't hesitate to point them out!

…ne from docblocks

alekitto · 2017-01-30T01:10:29Z

I made some changes, tomorrow i'll write an integration test.
The code looks quite readable and it was pretty easy to implement and find where to modify the Loader to call the new SimpleCaller class.
The only thing that was unclear to me is where to find services definitions for Symfony: I didn't understand that the caller was a service and not instantiated from the Loader. Have I missed something on the doc?

theofidry · 2017-01-30T01:19:41Z

The only thing that was unclear to me is where to find services definitions for Symfony: I didn't understand that the caller was a service and not instantiated from the Loader. Have I missed something on the doc?

The services can be found in src/Bridge/Symfony/Resources/config. Basically NativeLoader is a loader to be able to easily use alice out of the box, without any DIC. The Symfony bridge however ensures everything is registered as services to the Symfony DIC (via the bundle NelmioAliceBundle) to be able to take advantage of the features the DIC provides.

It's like a traditional bundle, but shipped here instead of a dedicated library (it's easier for maintenance on my end).

theofidry · 2017-01-30T17:59:11Z

@alekitto tell me when you need a last review, AFAICS there's only one comment left and should be ready to ship.

alekitto · 2017-01-30T21:16:57Z

Should be all done now

theofidry · 2017-01-30T21:58:09Z

Thanks very much @alekitto, that's a nice one :)

Implemented method calls via __calls

1742ab4

theofidry requested changes Jan 30, 2017

View reviewed changes

updated sf service definition, removed todo from doc and removed inli…

3d5ce28

…ne from docblocks

alekitto added 2 commits January 30, 2017 17:20

added a couple of integration tests

a905909

added call to setTitle in test as requested

94e333f

added reference value intergation test

631f5a5

theofidry approved these changes Jan 30, 2017

View reviewed changes

theofidry merged commit 1630700 into nelmio:master Jan 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented method calling via __calls #666

Implemented method calling via __calls #666

alekitto commented Jan 29, 2017

theofidry left a comment •

edited

Loading

theofidry Jan 29, 2017

theofidry Jan 29, 2017

theofidry Jan 29, 2017

theofidry Jan 29, 2017

theofidry Jan 29, 2017

theofidry Jan 29, 2017

theofidry Jan 29, 2017

theofidry Jan 29, 2017

theofidry Jan 30, 2017

theofidry Jan 30, 2017

alekitto Jan 30, 2017

theofidry Jan 30, 2017

theofidry commented Jan 30, 2017

alekitto commented Jan 30, 2017

theofidry commented Jan 30, 2017

theofidry commented Jan 30, 2017

alekitto commented Jan 30, 2017

theofidry commented Jan 30, 2017

Implemented method calling via __calls #666

Implemented method calling via __calls #666

Conversation

alekitto commented Jan 29, 2017

theofidry left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Jan 30, 2017

alekitto commented Jan 30, 2017

theofidry commented Jan 30, 2017

theofidry commented Jan 30, 2017

alekitto commented Jan 30, 2017

theofidry commented Jan 30, 2017

theofidry left a comment •

edited

Loading