Skip to content

feat(wiki): force lower case domain names on creation#613

Merged
m90 merged 1 commit into
mainfrom
fr/lowercase-domains
Jun 26, 2023
Merged

feat(wiki): force lower case domain names on creation#613
m90 merged 1 commit into
mainfrom
fr/lowercase-domains

Conversation

@m90

@m90 m90 commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

Ticket https://phabricator.wikimedia.org/T339860

I was able to test this locally, both with subdomains as well as custom domain names.

$this->route,
[
'domain' => 'derp.com',
'domain' => 'dErP.com',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Considering the amount of setup such a test case has to do I went for hogging on to the existng one. This could of course also be a standalone test case if we wanted it to.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nah, I think leaving it like this is fine

{
$user = $request->user();
$submittedDomain = $request->input('domain');
$submittedDomain = strtolower($request->input('domain'));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure about strings in PHP, does this need to be strtolower or mb_strtolower?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

either would do, I think there is no point in using mb_strlower which I assume is internally more expensive and complex.

This would be necessary if we were to go on to support multibyte chars in this place but since we filter any of those out with the validator I think it doesn't matter (and I don't think we'd ever want to support these in our domain names).

@m90
m90 force-pushed the fr/lowercase-domains branch from 318036e to c742a48 Compare June 21, 2023 12:37

@tarrow tarrow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good to me. l can confirm this works as I'd expect locally. :)

@m90
m90 merged commit 84ba095 into main Jun 26, 2023
@m90
m90 deleted the fr/lowercase-domains branch June 26, 2023 09:31
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