Skip to content
This repository has been archived by the owner on Jun 15, 2022. It is now read-only.

Thellimist/pi 1194 make unicode zones work by punycoding #148

Merged
merged 5 commits into from
Jun 28, 2017

Conversation

thellimist
Copy link
Contributor

I couldn't use idn-to-ascii function since php needs to be compiled with an extra flag. I assumed we don't have that access and downloaded a library which does it.

@thellimist thellimist requested review from manatarms and jwineman June 26, 2017 18:19
@@ -60,15 +61,21 @@ public function mergeCpanelAndCFDomains()
$cpanelDomainList = array_merge($cpanelDomainList, $getCpanelDomains['parked_domains']);
}

$Punycode = new Punycode();
Copy link
Contributor

Choose a reason for hiding this comment

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

new creates a hard coded dependency. Can we do getter/setter DI here and write a test to ensure this method handles unicode domains correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we ran into this issue on wordpress as well is there a possibility of moving this fix into cloudflare-plugin-backend so we ensure we handle unicode domains correctly across all plugins?

Copy link
Contributor Author

@thellimist thellimist Jun 26, 2017

Choose a reason for hiding this comment

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

The fix is dependent on GET /zone?name=something.com endpoint. cloudflare-plugin-backend only has generic calls. Max we can do is move the getter/setter of Punycode library there but I think it'd be better if each plugin has it separately unless we moveGET /zone?name=something.com to cloudflare-plugin-backend

}

public function setPunycoder($p = null) {
$this->punyCode = $p;
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we put this logic in the getter but it doesn't matter, this approach works as well.

@thellimist thellimist merged commit 59f78e4 into master Jun 28, 2017
@jwineman jwineman deleted the thellimist/PI-1194 branch July 13, 2017 17:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants