User tests: Successful: Unsuccessful:
Pull Request for Issue #36981
###Summary Of Changes
$searchEmail = '([\w.'-+]+@(?:[a-z0-9.-]+.)+(?:[a-zA-Z0-9-]{2,10}))';
This accepts only 10 characters due to which some emails were not picked by server and hence people were facing problem.
###Actual result BEFORE applying this Pull Request
Once the plugin(email-cloak) is activated, part of the TLD will be truncated.
Example:
office@some.international
becomes:
office@some.internatio
Expected result AFTER applying this Pull Request
E-mail will not be truncated
Status | New | ⇒ | Pending |
Category | ⇒ | Front End Plugins |
Maybe there is a regex from the rfc somewhere?
validating email with regex is almost impossible. There are as many regex available as there are stars in the sky. However the current regex is about as good as any and based on years of experience with real world examples. The object of this regex is NOT to validate an email address but to identify what the link says is an email address and cloaks it
The changes in this PR are NOT correct. The only thing that should be changed is the 10 character limit on the final part of the address.
This PR changes other parts of the regex that introduce new bugs. For example a test with the email address o'connor@example.com
original regex ==> o'connor@example.com
This regex ==> connor@example.com
I have tested this item
another valid email addresses that now fail is brian+text@example.com
original regex ==> brian+text@example.com
This regex ==> text@example.com
$searchEmail = '([\w\.\'\-\+]+\@(?:[a-z0-9\.\-]+\.)+(?:[a-zA-Z0-9\-]{2,18}))';
Sir, is this regex ok?
It is accepting all the emails you had mentioned including [office@some.international]
Labels |
Added:
?
|
$searchEmail = '([\w\.\'\-\+]+\@(?:[a-z0-9\.\-]+\.)+(?:[a-zA-Z0-9\-]{2,18}))';
Sir, is this regex ok? It is accepting all the emails you had mentioned including [office@some.international]
@shagungarg0 Your code change on GitHub doesn't show {2,18}
but it shows {2,345}
. Was that just for testing? If so please change, if not please explain. Beside that it seems to be ok now, i.e. nothing else change beside that value.
$searchEmail = '([\w\.\'\-\+]+\@(?:[a-z0-9\.\-]+\.)+(?:[a-zA-Z0-9\-]{2,18}))';
Sir, is this regex ok? It is accepting all the emails you had mentioned including [office@some.international]
@shagungarg0 Your code change on GitHub doesn't show
{2,18}
but it shows{2,345}
. Was that just for testing? If so please change, if not please explain. Beside that it seems to be ok now, i.e. nothing else change beside that value.
Sir , i had checked on google that longest email includes 345 characters so i increased its limit to 345 and it was working fine.
Can i create a new pull request?
Can i create a new pull request?
@shagungarg0 Why new pull request? For this one here? Or any other pull request for any other issue, not related to this one?
Can i create a new pull request?
@shagungarg0 Why new pull request? For this one here? Or any other pull request for any other issue, not related to this one?
For 4.0 branch
345 is the length of the full email not the TLD
345 is the length of the full email not the TLD
Sorry sir , I forgot. Tld is 24. wait a minute i will change it to 24.
For 4.0 branch
@shagungarg0 For the same thing as here? If the change would be the same for J4, it does not need to make a separate PR. The change would later be merged up from 3.10-dev into 4.1-dev. That's why I have closed your PR #36985 .
The 4.0-dev branch shall not be used anymore to make new PRs. The default branch is 4.1-dev now because the next stable version will be a 4.1.0.
In general when you make PRs, you should create a separate branch in your fork for each PR.
Now you are using your 3.10-dev branch for this PR and have used your 4.0-dev branch for the other PR.
This is ok as long as you make only one PR at a time for that target branch.
But what if you want to make another PR for something else, also for 3.10-dev? Then you have a problem because you cannot use your 3.10-dev which is already used for this PR here and so includes the changes for this PR here. And when you create a new branch for that new PR based on your 3.10-dev branch, it will also not be ok because that new branch will also include the changes from this PR here. So you would have to create that new branch based on the upstream's 3.10-dev branch to fix that.
Therefore, to avoid such struggle, you should always create a new branch when you want to make a new PR. The new branch should be based on the target branch in the upstream repository (= this one here) from your fork's point of view. And it helps to give the branches telling names. I personally always prepend my branch names with the name of the target branch, so I would call my branch for this PR here "3.10-dev-email-cloak" or something like that.
If you once get used to such methodology, you can safely work on several PRs for the same target branch at the same time without struggle and confusion.
Okk sir , i will take care of it and thank you for guidance.
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-02-20 09:24:36 |
Closed_By | ⇒ | zero-24 | |
Labels |
Added:
?
|
Merging thanks @shagungarg0 and the testers
From my understanding we should just remove the character limit for the domain part? Why do you want to change more things?
Maybe there is a regex from the rfc somewhere?