Skip to content

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Nov 5, 2014

As discussed in this thread: http://marc.info/?l=php-internals&m=141107203812897, this PR add IDN support in streams. The way it is done (by adding a new internal field in the php_url struct) will allow adding easily IDN validation in ext/filter.

TODO:

  • Write an RFC
  • Add Windows support

Tests must be enhanced, it would be great if the PHP org can setup for test purpose:

  • An HTTP, HTTPS and FTP enabled server with an IDN

@@ -16,6 +16,7 @@
| Jim Winstead <jimw@php.net> |
| Hartmut Holzgraefe <hholzgra@php.net> |
| Sara Golemon <pollita@php.net> |
| Kévin Dunglas <dunglas@gmail.com> |
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure edit author info for few lines change is okey...

Copy link
Member Author

Choose a reason for hiding this comment

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

@laruence removed except from url[.c|.h].

@auroraeosrose
Copy link
Contributor

HI - besides the mailing list - do you have an RFC in draft for this? This is adding a new library dependency for PHP core (ICU) which requires some additional discussion and it also doesn't have any of the pieces to compile on windows to even test. So it's definitely not even close to being ready to merge yet.

@dunglas
Copy link
Member Author

dunglas commented Nov 6, 2014

Hi @auroraeosrose, I've not written any RFC for now. I'll request access to do that.
I'll not have access to a Windows box until next week. I'll try to create missing pieces.

@dunglas
Copy link
Member Author

dunglas commented Nov 11, 2014

@auroraeosrose, I've requested RFC karma to php-webmaster but got no answer. Can you unlock my account? Or should I post the proposal here?

@auroraeosrose
Copy link
Contributor

Try mailing the php internals mailing list - also you just need wiki RFC access, so make sure you're asking for the right thing

@krakjoe
Copy link
Member

krakjoe commented Jan 2, 2017

Since this has merge conflicts and appears abandoned (because incomplete), and since we waited more than two years for an RFC to materialize, I'm closing this PR.

Please take this action as encouragement to create a clean PR, with satisfactory test coverage, a complete implementation, and start an RFC and accompanying discussion.

@krakjoe krakjoe closed this Jan 2, 2017
@dunglas
Copy link
Member Author

dunglas commented Jan 2, 2017

@krakjoe, the code was working and was complete for PHP 5. Not sure if it works for PHP 7 but I can take a look.

I did not take the time to write an RFC because according to the discussion it was very unlikely that a hard dependency to ICU will be accepted. But if you think that it can/should be discussed, I can take some time to create it.

@staabm
Copy link
Contributor

staabm commented Jan 2, 2017

@dunglas maybe you should start the discussion again on the maillinglist first, before putting any more effort into the code or the RFC.

@krakjoe
Copy link
Member

krakjoe commented Jan 2, 2017

@dunglas Not intimately familiar with discussion you mentioned, but if that is your impression then it is probably correct.

If you want to push forward, I would do as @staabm suggested and try to restart the discussion, try to gather consensus before putting more effort into code.

The PR could not have been used in it's current form, and I'm trying to do housework .... but don't take my closing the PR as "end of discussion", it of course isn't that ... just keeping house ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants