[BUG][QUESTION] UrlRoutable contract conflicts with Illuminate\Contracts\Routing\UrlRoutable

[dandrust-spark]

I'm trying to integrate Doctrine into my Laravel Project. I'm running into an issue where my entities must implement two UrlRoutable contracts for routing to work properly.

I have a Survey entity whose route key needs to be it's key property rather than it's id. In order for that to work I have o implement both UrlRoutable contracts:

• Implementing LaravelDoctrine\ORM\Contracts\UrlRoutable works in tandem with LaravelDoctrine\ORM\Middleware\SubstituteBindings to parse URLs for incoming requests.
• Implementing Illuminate\Contracts\Routing\UrlRoutable works in tandem with Illuminate\Routing\RouteUrlGenerator to generate URLs.

The issue is that both UrlRoutable contracts require implementation of getRouteKeyName. The LaravelDoctrine version calls for this method to be static, where the Illuminate version expects this to be a member function. A static and member function cannot be declared with the same name, and the LaravelDoctrine\ORM\Middleware\SubstituteBindings middleware explicitly checks to see if the interface is implemented on the Entity. So a fast-and-loose duck-typing situation won't work.

I can work around this by passing the entity's route key to the route helper in the special circumstance but it would be nice if the contracts shared the same function as they're designed for the same purpose.

Package version, Laravel version

laravel-doctrine/orm 1.6.0, laravel/framework v7.19.1

Expected behaviour

I would expect the getRouteKeyName method in LaravelDoctrine\ORM\Contracts\UrlRoutable to match Illuminate\Contracts\Routing\UrlRoutable

Actual behaviour

• LaravelDoctrine\ORM\Contracts\UrlRoutable declares public static function getRouteKeyName(): string
• Illuminate\Contracts\Routing\UrlRoutable declares public function getRouteKeyName()

Steps to reproduce the behaviour

<?php

namespace App\Entities;

use Illuminate\Contracts\Routing\UrlRoutable;
use LaravelDoctrine\ORM\Contracts\UrlRoutable as DoctrineUrlRoutable;

class Survey implements UrlRoutable, DoctrineUrlRoutable
{

    private $id;
    private $key;

    public static function getRouteKeyName() : string
    {
        return 'key';
    }

    public function getRouteKeyName()
    {
        return 'key';
    }

    // ...
}

[eigan]

How does your implementation of Illuminate\Contracts\Routing::UrlRoutable looks like? Seems like resolveRouteBinding would require the EntityManager?

Might it be easier to just extend Illuminate\Routing\UrlGenerator and use LaravelDoctrine\ORM\Contracts\UrlRoutable in UrlGenerator::formatParameters. Seems to be registered in RoutingServiceProvider::registerUrlGenerator. Doing this will make it unessary to implement Illuminate\Contracts\Routing\UrlRoutable, right?

[whizzrd]

Yes, resolveRoutebinding needs the entitymanager to resolve the entity e.g.

    public function resolveRouteBinding($value)
    {
        return EntityManager::getRepository(__CLASS__)->findOneBy([$this->getRouteKeyName() => $value]);
    }

my primary objective was to pass the entity as a route parameter, and \Illuminate\Routing\UrlGenerator::formatParameters only accesses \Illuminate\Contracts\Routing\UrlRoutable::getRouteKey

I now think that when implementing \Illuminate\Contracts\Routing\UrlRoutable on Entity one should use \Illuminate\Routing\Middleware\SubstituteBindings::class instead of \LaravelDoctrine\ORM\Middleware\SubstituteBindings::class

[eigan]

should use \Illuminate\Routing\Middleware\SubstituteBindings::class

I don't think you can, as laravel builds an instance of the model (entity) via the Container, which often is not possible whith doctrine entities. See https://github.com/illuminate/routing/blob/master/ImplicitRouteBinding.php#L36

[whizzrd]

Yes I see, in my trivial model without constructor it was actually working, however when adding a constructor that requires parameters the container can not resolve the parameter dependency

[douma]

Wouldn't the solution be as follows, or am I missing something?

Rename UrlRoutable static method:

<?php

namespace LaravelDoctrine\ORM\Contracts;
interface UrlRoutable extends \Illuminate\Contracts\Routing\UrlRoutable //extend here (this is optional, but maybe nicer solution)
{
 public static function getRouteKeyNameStatic(): string; //renamed this method 
}

The entity:

<?php

namespace App\Entities;

use Doctrine\ORM\EntityManager;
use Doctrine\ORM\Mapping as ORM;
use LaravelDoctrine\ORM\Contracts\UrlRoutable;
use Illuminate\Contracts\Routing\UrlRoutable as LaravelUrlRoutable;

/**
 * Users
 *
 * @ORM\Table(name="users", uniqueConstraints={@ORM\UniqueConstraint(name="users_email_unique", columns={"email"})})
 * @ORM\Entity
 */
class Users implements UrlRoutable
{
 public function getRouteKey()
 {
 return $this->email;
 }

 public function resolveRouteBinding($value)
 {
 return app()->em->getRepository(__CLASS__)->findOneBy([$this->getRouteKeyName() => $value]);
 }

 public static function getRouteKeyNameStatic(): string
 {
 return 'email';
 }

 public function getRouteKeyName(): string
 {
 return 'email';
 }

 //...... fields 

}

Modify SubstituteBindings.php:

    protected function substituteImplicitBindings(Route $route)
    {
        $parameters = $route->parameters();

        foreach ($this->signatureParameters($route) as $parameter) {
            $id    = $parameters[$parameter->name];
            $class = $parameter->getClass()->getName();

            if ($repository = $this->registry->getRepository($class)) {
                if ($parameter->getClass()->implementsInterface(UrlRoutable::class)) {
                    $name = call_user_func([$class, 'getRouteKeyNameStatic']); //here 

                    $entity = $repository->findOneBy([
                        $name => $id
                    ]);
                } else {
                    $entity = $repository->find($id);
                }

                if (is_null($entity) && !$parameter->isDefaultValueAvailable()) {
                    throw EntityNotFoundException::fromClassNameAndIdentifier($class, ['id' => $id]);
                }

                $route->setParameter($parameter->name, $entity);
            }
        }
    }

[eigan]

@douma Yes, that works. Though it is a breaking change :/

[dpslwk]

We should fix this at the same time we move to DABL3 since that's likely going to push us up a major version

[eigan]

Method renamed in 2.0