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

NotFoundHttpException if route parameters are 'encoded' #261

Closed
jatubio opened this issue Aug 13, 2017 · 6 comments
Closed

NotFoundHttpException if route parameters are 'encoded' #261

jatubio opened this issue Aug 13, 2017 · 6 comments

Comments

@jatubio
Copy link
Contributor

jatubio commented Aug 13, 2017

Hi Simon,

I'm using your library on one spanish web site.

For the route: /especialidad/{especialidad}
Sometimes i have one url like: /especialidad/cirugía local.
I have encoded the url and final url is: /especialidad/cirug%C3%ADa%20local, but i get NotFoundHttpException with this url.

In summary:

This works:
/especialidad/cirugia

This don't work:
/especialidad/cirug%C3%ADa (/especialidad/cirugía)
/especialidad/cirug%C3%ADa%20local (/especialidad/cirugía local)

Thank you

@skipperbent
Copy link
Owner

Hi @jatubio .

I will make some unit tests and see what I can do about the issue you are experiencing.

Until then you might be able to create your own workaround by defining your own regular expression for the parameters. By default the library uses only \w which matches A-Z, 0-9 and a limited set of special characters.

Please refer to the "Custom regex for matching parameters" section in the documentation for further information:
https://github.com/skipperbent/simple-php-router#custom-regex-for-matching-parameters

I think something like this could work (haven't tested):
[\w\%]+

I'll keep you posted when I've got further info.

/ Simon

@jatubio
Copy link
Contributor Author

jatubio commented Aug 13, 2017

Hi @skipperbent.

I have tested the url /cursos/listado/especialidad/Cardiolog%C3%ADa with SimpleRouter::('cursos/listado/{listado?}/{filtro?}', $callback, ['defaultParameterRegex' => '[\w\%]+']) but it don't work.

Maybe this can help you:

  • If i write /cursos/listado/especialidad/Cardiolog%C3%ADa, i get /cursos/listado/especialidad/Cardiología on php logs. Then, maybe you can add /u flag to parameter regex to support unicode letters.

But, it's not a complete solution because %20 for spaces.

Simon, Thank you for your time.

PS: If you think that it's too much work to solve this issue, I can change my code easily
to remove unicode characters and replace spaces with something like -. The main reason for opening this issue has been to help you to improve the library

@skipperbent
Copy link
Owner

Hi @jatubio .

Can you please try this regular expression the affected routes?
It will support spanish letters and accents, and in my testing it seem to pass on the routes you have issues with.

SimpleRouter::get('cursos/listado/{listado?}/{filtro?}', $callback, ['defaultParameterRegex' => '[\w\p{L}\s-]+']);

No worries - I always appreciate feedback which helps improve the project, so keep em' coming 👍

Let me know if the above works.

/ Simon

@skipperbent
Copy link
Owner

Copy-pasta of passing unit-test:

public function testUnicodeCharacters()
{
    TestRouter::get('/cursos/listado/{listado?}/{category?}', 'DummyController@method1', ['defaultParameterRegex' => '[\w\p{L}\s-]+']);
    TestRouter::get('/kategori/økse', 'DummyController@method1', ['defaultParameterRegex' => '[\w\ø]+']);

    TestRouter::debugNoReset('/cursos/listado/especialidad/cirugía local', 'get');
    $this->assertEquals('/cursos/listado/{listado?}/{category?}/', TestRouter::router()->getRequest()->getLoadedRoute()->getUrl());

    TestRouter::debugNoReset('/kategori/økse', 'get');
    $this->assertEquals('/kategori/økse/', TestRouter::router()->getRequest()->getLoadedRoute()->getUrl());

    TestRouter::router()->reset();
}

@jatubio
Copy link
Contributor Author

jatubio commented Aug 18, 2017

Thank you @skipperbent, it works like a charm! :)

@skipperbent
Copy link
Owner

I'm glad to hear. I'll close this issue.

/ Simon

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

No branches or pull requests

2 participants