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

[Fix] Mail: allow domains as domain.edu.co #4905

Merged
merged 5 commits into from
Oct 25, 2014

Conversation

infograf768
Copy link
Member

Former patches did not let enter emails of type myname@domain.edu.co (double dotted domains)

This will let them saved OK.

@infograf768 infograf768 changed the title [Fix} Mail: allow domains as domain.edu.co [Fix] Mail: allow domains as domain.edu.co Oct 23, 2014
@wilsonge
Copy link
Contributor

test: emails with 2 fullstops in domain work. A selection of other emails also seem to pass without any issues.

@MATsxm
Copy link

MATsxm commented Oct 23, 2014

@test

able to reproduce then #4905 works as expected with double dotted domains AND without like contact@test

thanks

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4905.

@infograf768
Copy link
Member Author

In fact this does not validate as @Domain is not accepted. See /tests/unit/suites/libraries/joomla/form/rule/JFormRuleEmailTest.php
( array('test3@localhost', false), )

@infograf768
Copy link
Member Author

I corrected the helper but the validation is still wrong, I guess because of the code in:
/Applications/MAMP/htdocs/trunkgitnew/libraries/joomla/form/rule/email.php
We could return false there if the domain does not contain a dot.

        $atIndex = strrpos($value, '@');
        $domain = substr($value, $atIndex + 1);

        // Check if domain contains a dot
        $domainDot = strstr($domain, '.');

        if ($domainDot === false)
        {
            return false;
        }

What do you think?
I am a bit confused by the code in /tests/unit/suites/libraries/joomla/form/rule/JFormRuleEmailTest.php

we have there
array('firstname@localhost', true)
array('test@example.com,test2@example.com,test3@localhost', true)
but
array('test3@localhost', false),

@infograf768
Copy link
Member Author

The question, in fact, is:
"Shall we authorise or not domains without a suffix?"

@infograf768
Copy link
Member Author

@MATsxm
Copy link

MATsxm commented Oct 24, 2014

"Shall we authorise or not domains without a suffix?"

I am not an expert but I don't see why we should not?
I may be wrong but it seems to me that this is even useful in some cases (I can't find anymore why but I think it has to do with local servers)

@brianteeman
Copy link
Contributor

It is very useful where Joomla is used on an intranet where they might use
internal addresses such as name@company
On 24 Oct 2014 11:17, "Marc-Antoine" notifications@github.com wrote:

"Shall we authorise or not domains without a suffix?"

I am not an expert but I don't see why we should not?
I may be wrong but it seems to me that this is even useful in some cases
(I can't find anymore why but I think it has to do with local servers)


Reply to this email directly or view it on GitHub
#4905 (comment).

@infograf768
Copy link
Member Author

If all agree, I will then revert
infograf768@b7621c2

and we just have to correct the validation to be always true in
/tests/unit/suites/libraries/joomla/form/rule/JFormRuleEmailTest.php

@wilsonge
Copy link
Contributor

I think there are valid reasons why these should not validate (e.g. http://www.symantec.com/connect/blogs/important-changes-ssl-certificates-intranets-what-you-need-know)

@phproberto
Copy link
Contributor

👍 to support domains without dots. We should not restrict email addresses more than the standards

@infograf768
Copy link
Member Author

Let's see if test pass this time. :)

@infograf768
Copy link
Member Author

WE still have

  1. JFormRuleEmailTest::testEmailData3 with data set Test #2 ('test3@example.c', false)

test3@example.c should have returned false but did not

Failed asserting that true matches expected false.

not passing.

@rdeutz
Copy link
Contributor

rdeutz commented Oct 24, 2014

looked into it and have send a PR to your branch JM

@infograf768
Copy link
Member Author

@rdeutz
Have you got a reference concerning the validity of domain.c ?

changed one condition to let the tests pass, splited one test in two
@infograf768
Copy link
Member Author

Merged Robert pull after discussion on skype

infograf768 added a commit that referenced this pull request Oct 25, 2014
[Fix] Mail: allow domains as domain.edu.co
@infograf768 infograf768 merged commit 9be642f into joomla:staging Oct 25, 2014
rvbgnu added a commit to rvbgnu/joomla-cms that referenced this pull request Nov 5, 2014
@rdeutz rdeutz added this to the Joomla! 3.4.0 milestone Feb 3, 2015
@@ -55,7 +55,7 @@ public function test(SimpleXMLElement $element, $value, $group = null, JRegistry

if ($tld)
{
$this->regex = '^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]{2,})$';
$this->regex = '^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$';
Copy link
Contributor

Choose a reason for hiding this comment

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

both regex a same now!

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost but not quite - look at the final few characters

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

None yet

8 participants