Laravel compatibility with RouteCollection::add()

[specialtactics]

Return Route from RouteCollection::add() for upstream compatibility with Laravel Router

1552

[catalinux]

is there a reason for not merging this pull request?

[specialtactics]

We need to get the package maintainers to do it I believe.

[catalinux]

ping @thilanga

[thilanga]

I'll go through the PR today and get it merge

On Thu., 9 Aug. 2018, 5:33 pm catalinux, notifications@github.com wrote:

ping @thilanga https://github.com/thilanga

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/dingo/api/pull/1555#issuecomment-411665445, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc_gj3hjjfQFMMpaRkkxumbvF45985wks5uO-XjgaJpZM4TvQ_9 .

[catalinux]

Great news. Thanks for all the effort!

[thilanga]

@catalinux @specialtactics Can I get this green before the merge please?

[specialtactics]

@thilanga Hey there, I had a look and it seems like there is a config issue with StyleCI - not sure I am able to fix that as it would require owners to fix it?

I will have a look at Travis, I assumed it was the same error but now see there is actually a failing test.

[catalinux]

@specialtactics I think it fails a test https://travis-ci.org/dingo/api/jobs/373878732

1) Dingo\Api\Tests\Routing\Adapter\LumenTest::testRoutingResources
TypeError: Argument 1 passed to Illuminate\Routing\RouteCollection::add() must be an instance of Illuminate\Routing\Route, null given, called in /home/travis/build/dingo/api/vendor/illuminate/routing/ResourceRegistrar.php on line 99
/home/travis/build/dingo/api/vendor/illuminate/routing/RouteCollection.php:50
/home/travis/build/dingo/api/vendor/illuminate/routing/ResourceRegistrar.php:99
/home/travis/build/dingo/api/src/Routing/Router.php:320
/home/travis/build/dingo/api/src/Routing/Router.php:299
/home/travis/build/dingo/api/tests/Routing/Adapter/BaseAdapterTest.php:195
/home/travis/build/dingo/api/src/Routing/Router.php:171
/home/travis/build/dingo/api/src/Routing/Router.php:136
/home/travis/build/dingo/api/tests/Routing/Adapter/BaseAdapterTest.php:197

[specialtactics]

@catalinux @thilanga Yep, I just had a look, it seems like the test actually fails regardless of this change or not - ie. If you just check out master of dingo/api and run the tests, this particular test will fail. So I am guessing there was an update which broke it in Laravel/Lumen?

I just a bit of a play around with it, and it seems like the basic problem is that there is a test for Lumen (the test failing), whereby a route is being added using the lumen adapter, but laravel code that is running is expecting a route object to be returned, where one won't be (because the lumen function for addRoute differs from the Laravel one in terms of not returning anything, whereas the Laravel function returns the route object).

I am not entirely sure how this situation came about - perhaps in the past Laravel also did not return the route, so nothing was expected there?

I think the test shouldn't be running through Laravel to the extent that it is, because in the call tree you can see Laravel and Lumen vendor functions being mixed, and this is the cause of the problem (ie. they are not compatible when it comes to routing at least).

Not sure what the next step is, I can try and work with it more, but would like to know a bit about the test setup and why it is doing what it's doing.

[specialtactics]

Actually after looking at it a bit further, I would suggest this package is just not compatible with Lumen anymore.

[gazben]

@specialtactics Yes, there was an upstream change: vendor/illuminate/routing/ResourceRegistrar.php

Old:

    public function register($name, $controller, array $options = [])
    {
        if (isset($options['parameters']) && ! isset($this->parameters)) {
            $this->parameters = $options['parameters'];
        }

        // If the resource name contains a slash, we will assume the developer wishes to
        // register these resource routes with a prefix so we will set that up out of
        // the box so they don't have to mess with it. Otherwise, we will continue.
        if (Str::contains($name, '/')) {
            $this->prefixedResource($name, $controller, $options);

            return;
        }

        // We need to extract the base resource from the resource name. Nested resources
        // are supported in the framework, but we need to know what name to use for a
        // place-holder on the route parameters, which should be the base resources.
        $base = $this->getResourceWildcard(last(explode('.', $name)));

        $defaults = $this->resourceDefaults;

        foreach ($this->getResourceMethods($defaults, $options) as $m) {
            $this->{'addResource'.ucfirst($m)}($name, $base, $controller, $options);
        }
    }

New:

    public function register($name, $controller, array $options = [])
    {
        if (isset($options['parameters']) && ! isset($this->parameters)) {
            $this->parameters = $options['parameters'];
        }

        // If the resource name contains a slash, we will assume the developer wishes to
        // register these resource routes with a prefix so we will set that up out of
        // the box so they don't have to mess with it. Otherwise, we will continue.
        if (Str::contains($name, '/')) {
            $this->prefixedResource($name, $controller, $options);

            return;
        }

        // We need to extract the base resource from the resource name. Nested resources
        // are supported in the framework, but we need to know what name to use for a
        // place-holder on the route parameters, which should be the base resources.
        $base = $this->getResourceWildcard(last(explode('.', $name)));

        $defaults = $this->resourceDefaults;

        $collection = new RouteCollection;

        foreach ($this->getResourceMethods($defaults, $options) as $m) {
            $collection->add($this->{'addResource'.ucfirst($m)}(
                $name, $base, $controller, $options
            ));
        }

        return $collection;
    }

I will look for a workaround.

[gazben]

@specialtactics If you add the old method to vendor/dingo/api/src/Routing/ResourceRegistrar.php it works. I will investigate, why this change was made, and where does the new return value go.

[gazben]

I sent the question to the original breaking change author, we are waiting for the answer. As a quick fix, reverting to the original method works see #1580

[gazben]

@thilanga After a bit of investigation I did not find any usage of the returned value in the laravel repo so I think the merge of #1580 is safe. The linting and unit tests are also fixed there, so if you first merge #1580 and after #1555 all checks will be green

[thilanga]

Beautiful. I'll do this as soon as I go to work.💪

On Thu., 16 Aug. 2018, 5:15 am Gazder Bence, notifications@github.com wrote:

@thilanga https://github.com/thilanga After a bit of investigation I did not find any usage of the returned value in the laravel repo so I think the merge of #1580 https://github.com/dingo/api/pull/1580 is safe. The linting and unit tests are also fixed there, so if you first merge #1580 https://github.com/dingo/api/pull/1580 and after #1555 https://github.com/dingo/api/pull/1555 all checks will be green

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dingo/api/pull/1555#issuecomment-413304829, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc_grIFwMHFCJfMAAuBT9yHSIjhPrrSks5uRHNigaJpZM4TvQ_9 .

[specialtactics]

Thanks guys! Sorry for slow follow-up, I was away