Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jun 8, 2016

By doing so one can

  • create a custom Router class inheriting from Slim\Router
  • redefine $container['router'] to return this custom Router instance
  • override createRoute method to return a custom Route (inheriting from Slim\Route)
  • add some extra methods in custom Route class (ex: getIsSecured())

…nherit from Slim\Route to add other methods: ex: getIsSecured()
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 96.605% when pulling f57f27a on vicolachips44:dev-3x into 30cfe3c on slimphp:3.x.

@marcelbonnet
Copy link

@vicolachips44 , I also was looking for a way to set my own implementation of \Slim\Route . Good one, I'll try it later.

But, @codeguy , @silentworks is this ok for merge? It would be much better to use this approach as a Slim official release, so we would not break compatibility in near future official releases, please!

@marcelbonnet marcelbonnet mentioned this pull request Jun 25, 2016
@akrabat
Copy link
Member

akrabat commented Jun 25, 2016

Why not just override map?

@marcelbonnet
Copy link

@akrabat , thanks for your question! See the piece of code below. IMHO, it push us to change to much core code just to insert a new function to the inherited class.

I am concerned about overriding map:

  • what if changes occour to Router::map in near future? It would break the approach
  • we have to create two new classes, an inheritance of Router and Route, because we do not have direct access to change the way Router instantiate a type of Route (see below)
  • and all this work just to instantiate another Route (like the example, RouteB) ?

That is why I agree with this merge request. Seems reasonable. Correct me if I'm wrong.

class RouteB extends \Slim\Route{

    public function allow($roles){
        //my new function
    }
}

class RouterB extends \Slim\Router {
    /**
     *
     * {@inheritDoc}
     * @see \Slim\Router::map()
     */
    public function map($methods, $pattern, $handler)
    {
        if (!is_string($pattern)) {
            throw new InvalidArgumentException('Route pattern must be a string');
        }

        // Prepend parent group pattern(s)
        if ($this->routeGroups) {
            $pattern = $this->processGroups() . $pattern;
        }

        // According to RFC methods are defined in uppercase (See RFC 7231)
        $methods = array_map("strtoupper", $methods);

        /* ******************************************************
                 * Is this approach ok just to instantiate RouteB instead of Slim\Route ????? 
                 *******************************************************
                */
        $route = new RouteB ($methods, $pattern, $handler, $this->routeGroups, $this->routeCounter); 
        $this->routes[$route->getIdentifier()] = $route;
        $this->routeCounter++;
        return $route;
    }
}

$container['router'] = new RouterB();

@akrabat
Copy link
Member

akrabat commented Jun 26, 2016

we have to create two new classes, an inheritance of Router and Route, because we do not have direct access to change the way Router instantiate a type of Route (see below)

You'll still have to do this.

@marcelbonnet
Copy link

You'll still have to do this.

but the point is: All we need is just to tell Router what kind of custom Route implementation we want. We don't want to change other parts of Router::map.

If Router::map make a call to a new function Router::createRoute($methods, $pattern, $callable) , it is done, no more map code replication :)

@akrabat akrabat added this to the 3.4.3 milestone Jul 26, 2016
@akrabat akrabat merged commit f57f27a into slimphp:3.x Jul 26, 2016
akrabat added a commit that referenced this pull request Jul 26, 2016
@akrabat akrabat modified the milestones: 3.5.0, 3.4.3 Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants