Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standard dispatch result #91

Merged
merged 6 commits into from
Oct 19, 2015
Merged

Standard dispatch result #91

merged 6 commits into from
Oct 19, 2015

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Oct 16, 2015

This PR implements a new class RouteInfo to return a standard result of disptach method.

This object describe current route resolved by dispatcher and helps us to implement new router libraries.

Reference #16

@gianarb gianarb added this to the 0.6.0 milestone Oct 16, 2015
$method = $routerInfo[1][1];
$function = (new ReflectionClass($controller))->getShortName();
if (!($routeInfo instanceof RouteInfoInterface)) {
throw new \RuntimeException('Dispatch does not return RouteInfo object');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move RuntimeException to use statement

@gianarb gianarb force-pushed the feature/route-info branch 5 times, most recently from 9652784 to d1cbd1c Compare October 17, 2015 18:31
RouteInfo is a new class that increase interoperability between
router libraries, this is the result of dispatch and it contains
all info necessary to complete the RUN
@gianarb gianarb force-pushed the feature/route-info branch from d1cbd1c to 2f8d26f Compare October 17, 2015 18:38
@gianarb
Copy link
Contributor Author

gianarb commented Oct 17, 2015

Works! :) I have only a little problem with PHP 7..

@fntlnz
Copy link
Contributor

fntlnz commented Oct 18, 2015

As you can see here PHP 5 and 7 don't behave in exactly the same way but both leads to an error if an array is passed instead of a callback.

I think that the problem is in how $fastRouteInfo[1][1] is populated here by the router.

@samsonasik
Copy link
Member

I've given a try to patch php7 issue in #92 , let see

@gianarb
Copy link
Contributor Author

gianarb commented Oct 18, 2015

This fix doesn't sound good but works..
Idea?

@samsonasik
Copy link
Member

seems weird as previously, [$controller, $method] should be callable, but probably can be a workaround solution for now ;)

@samsonasik
Copy link
Member

btw, need additional tests

Gianluca Arbezzano added 2 commits October 19, 2015 09:35
Feature/route info call user func approach
@gianarb
Copy link
Contributor Author

gianarb commented Oct 19, 2015

Thanks at all! We have a RouteInfo! 👍

gianarb pushed a commit that referenced this pull request Oct 19, 2015
@gianarb gianarb merged commit b1779e3 into master Oct 19, 2015
@gianarb gianarb mentioned this pull request Oct 19, 2015
private $callable;
private $params;

public static function matched($name, callable $callable, $params = [])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this method?

public function __construct($name, callable $callable, $params = []) {
    // ...
}

Is not better? In that way you can simplify the RouteInfoInterface and let to the user the RouteInfo construction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$matched = new RouteInfo("name", function() {}, []);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.. maybe it is more easy to use 💣 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion also because a match method is more related to the Router and not the matched route, do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :).. are you interested to write a PR? Otherwise I write a PR this evening :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course i will try with a new PR


interface RouteInfoInterface
{
public static function matched($name, callable $callable, $params);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no reason to have this method in the RouteInfoInterface imho.

@gianarb gianarb deleted the feature/route-info branch October 25, 2015 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants