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

Update Route matches #583

Closed
wants to merge 2 commits into from
Closed

Update Route matches #583

wants to merge 2 commits into from

Conversation

WinterSilence
Copy link

Reduced code. Fixed uppercase for params (controller, category).

Reduced code. Fixed uppercase for params (controller, category).
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.

@acoulton
Copy link
Member

acoulton commented Dec 8, 2014

Thanks for this, it needs unit tests particularly to show what functionally changes (I think just that it now supports directory params including \ or /?).

Also the commit message is not very clear, it's not enough to say "Fixed xxx", we need to know what about them wasn't right before and is now. Did you fix a bug, fix performance, improve implementation without changing behaviour? If it's a reported issue you should reference it, if not please include a full bug/issue description in the commit or PR.

@WinterSilence
Copy link
Author

I think just that it now supports directory params including \ or /?

$params['directory'] = ucwords(str_replace(array('\\', '/'), ' ', $params['directory']));
$params['directory'] = str_replace(' ', DIRECTORY_SEPARATOR, $params['directory']);

If it's a reported issue you should reference it, if not please include a full bug/issue description in the commit or PR.

My main problem is English translation (( It takes more time than writing code.

At first, we need to discuss whether changes are needed? After this, make adjustments and
test final version.

@@ -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

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