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

Allow capitalized usernames if the regex agrees #2138

Closed
wants to merge 5 commits into from
Closed

Allow capitalized usernames if the regex agrees #2138

wants to merge 5 commits into from

Conversation

newbthenewbd
Copy link
Contributor

I don't know the exact motivation behind these uses of strtolower, but I believe that the existence of the username regular expression in Grav's config clearly implies that the usernames don't just have to be lowercase, hence this pull request.

Copy link
Member

@mahagr mahagr left a comment

Choose a reason for hiding this comment

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

Filenames are case insensitive in most systems and removing strtolower() will just mess up things and cause issues especially if you have Win/Mac development system and Linux server.

@newbthenewbd
Copy link
Contributor Author

newbthenewbd commented Aug 8, 2018

Right, completely forgot about this limitation, I apologize.

Still, the way this currently is implemented, Grav noticeably lowers its chances of being used in any sort of social platform, except possibly one about retro computing, limiting its usage to great-looking, high-performance blogs. Due to that, I believe this should still be somehow fixed.

The best implementation I can spontaneously think of is to prepend the usernames with the character case masks, in an optimal number system, i.e. 2002_xx13375h0073rxx for xX13375h0073rXx because hexadecimal 2002 is binary 010000000000010. That shouldn't matter at all for websites which only ever have a few admins registered, while the slightly increased disk usage is certainly a better solution for those who want more. Note also that this seems like a naive implementation, as the numbers could visibly be omitted here, however the intuitively better one might use too much CPU time in the lovely language PHP is, hence needing some tests.

Due to that, I request that this pull request stays open for now, with higher quality commits hopefully appearing soon.

@mahagr
Copy link
Member

mahagr commented Aug 8, 2018

I would just have field username inside the yaml, which overrides the one in filename if set.

@mahagr
Copy link
Member

mahagr commented Aug 8, 2018

Also adds support for Unicode in the usernames, and patches a nasty bug where the usernames don't get saved lowercase.
@newbthenewbd
Copy link
Contributor Author

Please review it again. Note that the bug about usernames not getting saved lowercase has been patched by removing the !$file->filename() conditional, which seemed to erroneously return false at all times, however I'm admittedly not exactly sure what it was supposed to do in there.

Thanks!

@newbthenewbd
Copy link
Contributor Author

Whoops, forgot about that one (and totally haven't woken up suddenly realizing this), here we go, hopefully.

@mahagr
Copy link
Member

mahagr commented Aug 14, 2018

Looks much better now. The only thing which I don't like is to remove the filename check as it causes filename to potentially change after every save thus causing the user to duplicate.

@newbthenewbd
Copy link
Contributor Author

Closed due to the general mess in this pull request's commits and the release of Grav 1.5.0. Should totally make a new pull request for the develop branch Soon™.

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.

2 participants