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

Ensure valid route alias names #243

Closed
wants to merge 1 commit into from
Closed

Conversation

xfra35
Copy link
Member

@xfra35 xfra35 commented Feb 2, 2018

I think that the regex in $f3->route() is a bit too permissive for aliases. It allows nearly all characters, in particular dots, while it should stick to the \w range.

What drew my attention is an issue for my f3-access plugin where the user has defined aliases with dots: xfra35/f3-access#4

Additionally, there's that ? after @ which seems superfluous to me. But I could be missing something.

@ikkez
Copy link
Member

ikkez commented Feb 10, 2018

We had \w+ before, and the change to .+? was introduced not that far ago and it seems to be made on purpose, see this blame: a27f2f0
Perhaps it was also regarding some unicode support 🤔

@xfra35
Copy link
Member Author

xfra35 commented Feb 10, 2018

You're right, thanks.

@bcosca what was the reason of the tweak in route() regex in commit a27f2f0?

Was it to allow unicode characters in route aliases? If yes, we should forbid dots and make the regex consistent in mock() as well as in the second part of the route() regex.

@bcosca
Copy link
Collaborator

bcosca commented Feb 10, 2018

You're absolutely right. The intent of that commit was to allow for unicode characters.

@ikkez ikkez changed the title Cleanup $f3->route() regex Ensure valid route alias names Nov 14, 2019
ikkez added a commit that referenced this pull request Nov 14, 2019
@ikkez
Copy link
Member

ikkez commented Nov 14, 2019

fixed in 5164e8c

@ikkez ikkez closed this Nov 14, 2019
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