-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support rfc6530 #5237
base: trunk
Are you sure you want to change the base?
Support rfc6530 #5237
Conversation
18ce341
to
c8587f1
Compare
I don't understand the test failures. They appear to be unrelated to these code changes. Could you take a look, @ironprogrammer ? Thanks. |
In response to @arnt:
Absolutely, please do add unit tests (or updates to existing tests) for
It's not uncommon for E2E test timeouts to occur in this repo's automated jobs. These can be ignored. |
Thanks for the updated PR! The updates to This isn't a formal test report (there are lots of scenarios needed), but I ran through a few smoke tests so as to get back to you quickly on what could be addressed next. It would be great to get additional reviewers/testers involved to cover what I've missed:
|
Sigh. I see what you mean. My own focus is/was quite different: I wanted Contact Form 7 and similar to accept unicode email addresses, which requires some utility functions in Wordpress to do the right thing. I didn't aspire to complete support in Wordpress. The thinking behind your response seems to be that either Wordpress supports it properly or not at all, there's no inbetween. And, sadly, I think you're right and I was wrong. I'll extend the PR. It'll take a few weeks. I'll also find some testers. |
Hi, you asked for wider testing. Around 20 sites have now run the patch in production, the longest-running one for more than six months already. Nothing has broken. The sites are real (University of Somewhere, not someone's private five-visitor site). If you want, I can send you the email addresses of some testers. When I pinged them last week, around half said it was okay to give you their addresses, I'm sure most will say the same if I ask a few times ;) |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I liked the accounts earlier today. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Thanks for the update, @arnt, and for seeking wider testing! There are still some outstanding questions and one previous failure to consider, but it's great to see this moving forward! 🙌🏻 A good next step would be to rebase this PR on the latest |
Finding testers who were willing and able to run a patch on sites that mattered to them was hard work. That took a good long while. Do you want their names? If so, please send me mail. Rebasing is not a problem, of course. The other stuff was a little difficult. I only recently found a good workaround for IntlChar's lack of support for UAX24. Can you suggest a model end-to-end test that I could emulate and write automatic tests for the desired features? Manual testing is possible, but I'd much prefer to have complete automatic test coverage. |
This adds support for the unicode address extensions in RFC 6532, adds unit tests for that, extends the documentation to explain the relationship between this code and the various specifications, and finally adds unit tests to ensure that the documentation's description of the code remains correct. Fixes #31992.
Related to #31992.
Hi, the remaining problems, as far as I can tell:
|
This accepts user names that contain a single script, but not mixed-script names, such as ones that mix Latin and Cyrillic. That seemed to be closest to the code's existing philosophy. Since PHP 7.2 and 7.3 don't offer mb_str_split, this change leaves sites on those versions of PHP with ASCII-only user names. Existing user names should continue to work unchanged, adding new (non-ASCII) user names will not work.
Until now, WordPress generated all-ASCII slugs, which works well for mostly-ASCII languages. Now that WordPress supports scripts like Devanagari much better, it's necessary to test the slug support for Devanagari. What WordPress did until now works well for Devanagari, so this commit merely adds a unit test. The ideal is to have links such as <hebrewdomain>.<hebrewtld>/<hebrewpath> which is shown onscreen as <hebrewpath>/<hebrewtld>.<hebrewdomain> without any switches between right-to-left Hebrew and left-to-right Latin. /author/ and a few others don't achieve that ideal. Yet. Note that there is a rare existing bug: The existing mapping to ASCII can produce the same slug for two pages, since two letters can map to e.g. 'L'. This seems unfixable, since any change might break existing links, but happily also extremely rare.
I added some code to prevent PHP 7.2.24-7.3.x from choking on mb_str_split(). A bit on testing:
And… oh drat… Must retest the site setup wizard, too much code has changed. Two questions remain.
|
Just set up a new site from scratch using 'rød@grå.org' as admin address; it works. |
Hi @ironprogrammer, would you mind having a look? Also, on an unrelated matter, I'm with you. I've worked as a full-time maintainer on a widely used open source project and got a lot of flak. It hurt. Some people can be really shitty. Don't let it grind you down. |
This adds support for unicode email addresses in is_email and sanitise_email. It is intended to be sufficient to accept unicode email addresses in contact forms etc.,
it does not aspire to unicode support everywhere, in particular it doesn't deal with unicode in logins or passwords.
It also does not add a tests/formatting/sanitizeEmail.php; I wouldn't mind adding one, but don't quite understand your testing policies. It looks like you want to keep the tests fast by avoiding unnecessary tests, I'd say, but I don't understand what you consider necessary and unnecessary. Anyway, if you want a file I'm happy to add one.
Trac ticket: https://core.trac.wordpress.org/ticket/31992