Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

zf-rpc fails for controller classes with same name in callable #18

Closed
Wilt opened this issue Dec 19, 2018 · 10 comments
Closed

zf-rpc fails for controller classes with same name in callable #18

Wilt opened this issue Dec 19, 2018 · 10 comments

Comments

@Wilt
Copy link

Wilt commented Dec 19, 2018

I am trying to register classes according to the example in the documentation, where the (controller) class name used as the key in the zf-rpc configuration and the class name for the callable are corresponding (in the example below Application\Controller\LoginController).

'Application\Controller\LoginController' => [
    'http_methods' => ['POST'],
    'route_name'   => 'api-login',
    'callable'     => 'Application\Controller\LoginController::process',
],

The application ends in a infinite loop :

Zend\ServiceManager\ServiceManager->get( )	                        ...\AbstractPluginManager.php:141
Zend\ServiceManager\ServiceManager->doCreate( )	                        ...\ServiceManager.php:200
ZF\Rpc\Factory\RpcControllerFactory->__invoke( )	                ...\ServiceManager.php:764
ZF\Rpc\Factory\RpcControllerFactory->marshalCallable( )	                ...\RpcControllerFactory.php:84
ZF\Rpc\Factory\RpcControllerFactory->marshalCallableFromContainer( )	...\RpcControllerFactory.php:122
Zend\ServiceManager\AbstractPluginManager->get( )                       ...\RpcControllerFactory.php:158

And this process repeats...

I will provide a test case that shows this problem in a forked repository here:
https://github.com/Wilt/zf-rpc/tree/issue18

Expected results

It should simply create the RpcController instance.

Actual results

Infinite loop.

Solution

I could provide a solution in a pull-request, but I need some guidance in where I preferably should change the code base for this.

Wilt pushed a commit to Wilt/zf-rpc that referenced this issue Dec 19, 2018
@weierophinney
Copy link
Member

What does the route definition look like?

@Wilt
Copy link
Author

Wilt commented Dec 20, 2018

In the test I didn't define a route, that was not necessary to reproduce the problem.
But a route would look for example like:

'login' => array(
    'type' => 'literal',
    'options' => array(
        'route' => '/login',
        'defaults' => array(
            'controller' => 'Application\Controller\LoginController'
        )
    )
),

@weierophinney
Copy link
Member

How is it not necessary to reproduce the problem? How exactly are you fetching the controller, if not when routing?

Honestly, I'm having trouble understanding the full context of the problem. You're reporting something that looks like a cyclical dependency lookup, but I'm not sure how the lookup is being triggered — which is why I asked about routing. And that routing example looks problematic: the name doesn't match what you wrote above (login vs api-login).

Update

In looking at this more closely, it looks like the OptionsListener is correctly identifying that an RPC controller was requested... however, the logic for marshalling the RpcController instance with the configured callable... is missing. I'll look into this more closely at the start of the year.

@Wilt
Copy link
Author

Wilt commented Dec 20, 2018

Maybe I was not clear in my question, but I provided a unit test that reproduces the problem in an test case. What I meant was that setting a route was not necessary to reproduce the issue in this unit test.
A link to a forked repository where I added the test is provided in the question.

Thanks for looking into this issue.
Happy holidays!

@weierophinney
Copy link
Member

@Wilt I had indeed missed the test case! (it's in test/Factory/RpcControllerFactoryTest.php for those wondering; last test, labelled testFailsWhenUsingRealControllerManagerWithAbstractFactory)

I can verify that the test never completes, which mimics the behavior you reported. Curious to see if I can figure out why now!

@weierophinney
Copy link
Member

I figured out what's going on, but I'm not sure how to fix it.

Essentially, what happens is this: the abstract factory splits the callable entry into a class and a method. It then tries first to see if the ControllerManager can produce an instance for that service name, and then the parent container, defaulting to attempting to instantiate directly.

The problem is this: when the class name is passed to the ControllerManager, if there is no entry for the class as an alias, factory, or invokable, it then hits the abstract factories... and the abstract factory decides it can produce an instance for it (because an entry exists for that class in the zf-rpc configuration, after all!), and the manager delegates to it... and so it attempts to pass the same name again to the ControllerManager, and we hit an infinite loop.

I'm totally stymied at how we might correct this. The only thing I can think of is a userland workaround: use a different name for the zf-rpc entry and the callable it would resolve to.

This could work in one of two ways:

  • Use a non-existent class for the zf-rpc configuration (which would be referenced in the routing), and a real class in its callable option:

    $config = [
        'zf-rpc' => [
            TestAsset\FooBar::class => [ // NOT A REAL CLASS
                'callable' => TestAsset\Foo::class . '::bar',
            ],
        ],
    ];
  • Use a different service name in the callable that resolves to a defined service:

    $config = [
        'controllers' => [
            'factories' => [
                ActualServiceController::class => ItsFactory::class,
            ],
        ],
        'zf-rpc' => [
            TestAsset\Foo::class => [
                'callable' => ActualServiceController::class . '::bar',
            ],
        ],
    ];

I might be able to do a circular-dependency check, but the ideas I have so far have some definite short-comings, particularly when working in async application runners, so I need to test a bit more.

weierophinney added a commit to weierophinney/zf-rpc that referenced this issue Jan 7, 2019
As reported in zfcampus#18, a circular dependency lookup can occur when
specifying a static method of a class as a `callable` for an RPC service
that matches the class name for the RPC service. As an example:

```php
return [
    'zf-rpc' => [
        SomeController::class => [
            'callable' => SomeController::class . '::bar',
        ],
    ],
];
```

In the above case, the `RpcControllerFactory` would be invoked for
`SomeController::class`, find the `callable` entry of
`{NS}\SomeController::bar`, split this into the class and method, and
then attempt to fetch `SomeController::class` again, ad infinitum.

This patch adds a private property, `$lastRequestedControllerService`,
which is set to the service being pulled whenever we attempt to marshal
a service from the controller manager. If a subsequent invocation finds
that the service requested matches that property value, then we do not
attempt to pull from the controller manager again, but move on to the
parent container (if we have one), or directly instantiating the class.
(The `$lastRequestedControllerService` is reset otherwise.)

Fixes zfcampus#18
weierophinney added a commit to weierophinney/zf-rpc that referenced this issue Jan 7, 2019
As reported in zfcampus#18, a circular dependency lookup can occur when
specifying a static method of a class as a `callable` for an RPC service
that matches the class name for the RPC service. As an example:

```php
return [
    'zf-rpc' => [
        SomeController::class => [
            'callable' => SomeController::class . '::bar',
        ],
    ],
];
```

In the above case, the `RpcControllerFactory` would be invoked for
`SomeController::class`, find the `callable` entry of
`{NS}\SomeController::bar`, split this into the class and method, and
then attempt to fetch `SomeController::class` again, ad infinitum.

This patch adds a private property, `$lastRequestedControllerService`,
which is set to the service being pulled whenever we attempt to marshal
a service from the controller manager. If a subsequent invocation finds
that the service requested matches that property value, then we do not
attempt to pull from the controller manager again, but move on to the
parent container (if we have one), or directly instantiating the class.
(The `$lastRequestedControllerService` is reset otherwise.)

Fixes zfcampus#18
@weierophinney
Copy link
Member

@Wilt Created a fix in #19.

@Wilt
Copy link
Author

Wilt commented Jan 15, 2019

I finally managed to find some time to continue working with this and now I noticed that I get a double wrapped callable as a controller from the RpcControllerFactory:

class ZF\Rpc\RpcController#1601 (7) {
  protected $wrappedCallable =>
  array(2) {
    [0] =>
    class ZF\Rpc\RpcController#1615 (7) {
      protected $wrappedCallable =>
      array(2) {
        ...
      }

The callable inside the RpcController instance is another instance of the RpcController and inside the nested instance the correct wrapped callable can be found.
As a result the method bar is called on the RpcController instance resulting in a "Method bar does not exist" error message.

It seems like the fix solved the circular dependency lookup but wraps the result 2 times.
I added some extra lines to the test to show where it fails and for demonstration purposes I added some additional lines to show when the exception is thrown:

Repository:
https://github.com/Wilt/zf-rpc/tree/issue18

Added to test:
https://github.com/Wilt/zf-rpc/blob/issue18/test/Factory/RpcControllerFactoryTest.php#L333-L351

@weierophinney
Copy link
Member

@Wilt Can you send a PR with the test case, please?

@Wilt Wilt mentioned this issue Jan 15, 2019
1 task
@Wilt
Copy link
Author

Wilt commented Jan 15, 2019

Done, can be found here: #20

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

No branches or pull requests

2 participants