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.

Sign up for GitHub

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

Update Route matches #583

Closed
wants to merge 2 commits into from
Closed

Update Route matches #583

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a363698
Update Route::matches()
WinterSilence Dec 7, 2014
0209847
Update Route.php
WinterSilence Dec 8, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 28 additions & 19 deletions classes/Kohana/Route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,9 +397,8 @@ public function filter($callback)
}

/**
* Tests if the route matches a given Request. A successful match will return
* all of the routed parameters as an array. A failed match will return
* boolean FALSE.
* Tests if the route matches a given [Request]. A successful match will return
* all of the routed parameters as an array, a failed match will return FALSE.
*
* // Params: controller = users, action = edit, id = 10
* $params = $route->matches(Request::factory('users/edit/10'));
Expand All @@ -411,34 +410,29 @@ public function filter($callback)
* // Parse the parameters
* }
*
* @param Request $request Request object to match
* @return array on success
* @return FALSE on failure
* @param Request $request Request object to match
* @return mixed
*/
public function matches(Request $request)
{
// Get the URI from the Request
$uri = trim($request->uri(), '/');

if ( ! preg_match($this->_route_regex, $uri, $matches))
if ( ! preg_match($this->_route_regex, $uri, $params))
return FALSE;

$params = array();
foreach ($matches as $key => $value)
foreach ($params as $key => $value)
{
// Delete all unnamed keys
if (is_int($key))
{
// Skip all unnamed keys
continue;
unset($params[$key]);
Copy link
Member

Choose a reason for hiding this comment

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

It might be clearer and faster to use array_filter here?

Copy link
Author

Choose a reason for hiding this comment

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

To do this need create closure or there is basic function?

Copy link
Member

Choose a reason for hiding this comment

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

Well is_string is probably the opposite of is_int for this case?

But a closure with ! is_numeric might be safer to be sure it filters "1" etc:

$params = array_filter(
    $matches, 
    function($key) { return ! is_numeric($key); },
    ARRAY_FILTER_USE_KEY
);

Copy link
Author

Choose a reason for hiding this comment

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

your code call function -> function create object -> function call object in cycle,
how it can work faster?

Copy link
Member

Choose a reason for hiding this comment

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

array_filter is often a lot faster than anything with foreach, because the loop and the data management all happens in native code - there are many fewer userland operations and stack changes. You're right I'd not allowed for the overhead of the closure though which is higher than I'd expected.

We can't use my example above because ARRAY_FILTER_USE_KEY is PHP >= 5.6. For lower versions the best I've found is:

$int_keys = array_filter(array_keys($matches), 'is_numeric');
$params   = $int_keys ? array_diff_key($matches, array_fill_keys($int_keys, NULL)) : $matches;

I just did a quick benchmark of:

  • the original code (foreach and copy valid elements to new array)
  • your code (foreach and unset invalid)
  • array_filter with native function (can't use)
  • array_filter with closure (can't use)
  • possible solution
4 string 3 string + 1 int 13 string 10 string + 3 int
copy valid 24 μs 22 μs 69 μs 68 μs
unset invalid 20 μs 19 μs 58 μs 58 μs
array_filter native 6 μs 6 μs 7 μs 8 μs
array_filter closure 37 μs 36 μs 106 μs 102 μs
array_filter + diff_key 11 μs 19 μs 14 μs 22 μs

So it's basically always worth doing array_filter if there's a native function that does what you need, or if you can use a native function to find the values to reject and then array_diff_key to strip them out of the array.

Note also that it's fastest in the common (pure associative array) case, with only a small hit if it finds integer keys to remove.

}

// Set the value for all matched keys
$params[$key] = $value;
}

foreach ($this->_defaults as $key => $value)
{
if ( ! isset($params[$key]) OR $params[$key] === '')
if ( ! isset($params[$key]) OR empty($params[$key]))
{
// Set default values for any key that was not matched
$params[$key] = $value;
Expand All @@ -447,14 +441,27 @@ public function matches(Request $request)

if ( ! empty($params['controller']))
{
// PSR-0: Replace underscores with spaces, run ucwords, then replace underscore
$params['controller'] = str_replace(' ', '_', ucwords(str_replace('_', ' ', $params['controller'])));
if (strpos($params['controller'], '_') !== false)
Copy link
Member

Choose a reason for hiding this comment

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

Allcaps FALSE

{
// PSR-0: Replace underscores with spaces, run ucwords, then replace underscore
$params['controller'] = str_replace(' ', '_', ucwords(str_replace('_', ' ', $params['controller'])));
}
else
{
$params['controller'] = ucfirst($params['controller']);
}
}

if ( ! empty($params['directory']))
{
// PSR-0: Replace underscores with spaces, run ucwords, then replace underscore
$params['directory'] = str_replace(' ', '_', ucwords(str_replace('_', ' ', $params['directory'])));
$params['directory'] = ucwords(str_replace(array('\\', '/'), ' ', $params['directory']));
$params['directory'] = str_replace(' ', DIRECTORY_SEPARATOR, $params['directory']);

if (strpos($params['directory'], '_') !== false)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this also require an else block with ucfirst? This replacement seems to be duplicate for directory and controller, could you extract a method?

I think this doesn't actually change behaviour, just saves some operations if there's no _ in the param value? Might be even clearer/more efficient to do something like:

$param = preg_replace_callback(
    '/(^|_)\w/', 
    function($matches){return strtoupper($matches[0]);},
   $param
);

Perhaps worth benchmarking?

Copy link
Author

Choose a reason for hiding this comment

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

preg_* functions are slower, function "faster" objects (closure).
Need to fix code #571 ?

Copy link
Member

Choose a reason for hiding this comment

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

Sure preg_* and callback has a penalty, but multiple strpos, ucwords, str_replace will also add up - they all have to scan the full string and switch between native and PHP repeatedly. Worth checking I think because it would definitely make the code much more readable to get it in a single operation.

Copy link
Author

Choose a reason for hiding this comment

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

code for #571

if (!empty($params['namespace'])) {
    if (strpos('\\', $params['namespace']) !== false) {
        $params['namespace'] = str_replace(' ', '\\', ucwords(str_replace('\\', ' ', $params['namespace'])));
    } else {
        $params['namespace'] = ucfirst($params['namespace']);
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

@acoulton also, your code create closure object at each call Route::match(), its wrong - need create closure only once and contain in protected property of route. Now Route::match() use something like this, but need fix it.

Copy link
Member

Choose a reason for hiding this comment

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

@WinterSilence #571 won't be merged, #578 is the implementation for that which is going to be much simpler.

My snippet with closure was just for example, yes obviously you'd probably optimise/tidy up for implementation.

{
// PSR-0: Replace underscores with spaces, run ucwords, then replace underscore
$params['directory'] = str_replace(' ', '_', ucwords(str_replace('_', ' ', $params['directory'])));
}
}

if ($this->_filters)
Expand All @@ -473,10 +480,12 @@ public function matches(Request $request)
{
// Filter has modified the parameters
$params = $return;
// @todo if $return != $params, then uppercase values (directory, controller)
Copy link
Member

Choose a reason for hiding this comment

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

I think if filters return controller/directory etc they should be responsible for returning in proper case. The only reason we do the case transformation is so that URLs can be lowercase. Application code should be case-sensitive just like everywhere else.

In fact one use for a filter would be to transform a URL to some different casing convention eg mysql to mySQL rather than Mysql so I think we shouldn't mess about with the return.

Also if you are introducing @todos in a PR please could you flag in the description so it's clear it's not ready to merge?

Copy link
Author

Choose a reason for hiding this comment

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

Also if you are introducing @todos in a PR please could you flag in the description so it's clear it's not ready to merge?

How do it?

Copy link
Member

Choose a reason for hiding this comment

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

For example name the PR "[WIP] Update Route matches", and if it's @todo because you're not sure then in the description or as a line comment write something like "Question: should we uppercase controller/directory again after filters have run?"

Copy link
Author

Choose a reason for hiding this comment

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

ok, thanks add it in new PRs.

}
}
}

// @todo throw exception then some params not exists (controller, action)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that covered already in the request client?

Copy link
Author

Choose a reason for hiding this comment

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

I did not find it in request

Copy link
Member

Choose a reason for hiding this comment

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

My mistake, assumed it was but you're right so yes we should throw here if required params are missing.

return $params;
}

Expand Down