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

Question - Why is a - classified as an invalid slug character? #1459

Closed
rhubarbselleven opened this issue May 2, 2017 · 9 comments
Closed

Comments

@rhubarbselleven
Copy link

See: https://github.com/getgrav/grav/blob/develop/system/src/Grav/Common/Page/Page.php#L1500-L1502

I would have thought that a - would be a valid character to use in a slug.

@mahagr
Copy link
Member

mahagr commented May 2, 2017

It is valid, but not as the first character.

@rhubarbselleven
Copy link
Author

Ah, I see the issue. It's enforcing a lowercase slug. Should the slug not be lowercase then this notice is raised.

Should this be the case? Is the direction of the team to only accept lower case slugs? Further, does this imply that only lowercase ascii is supported?

@mahagr
Copy link
Member

mahagr commented May 3, 2017

Not sure. I think the decision to use lower case slugs was made because of Linux filesystem is case-sensitive and it would cause too many issues if we allowed people to use mixed-case in Windows and then deploy their sites in Linux server.

@rhubarbselleven
Copy link
Author

Generated slugs, sure I can understand that. But where a slug is set explicitly, I wouldn't expect this.

@mahagr
Copy link
Member

mahagr commented May 4, 2017

@rhukster What do you think?

@flaviocopes
Copy link
Contributor

Actually this just adds an entry in the log, and things still work. I aligned that to the admin-side slug regex, also adding support for Cyrillic charset

@rhubarbselleven
Copy link
Author

But when you have 2.3k pages that fail this on this check, that creates a huge amount of logs and so is not just an entry in a log but a larger capacity and scale issue

@rhukster
Copy link
Member

rhukster commented May 4, 2017

We can remove the logging statement, it really doesn't serve much purpose.

@rhukster
Copy link
Member

rhukster commented May 4, 2017

done.

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

4 participants