RTC PR-3.10-dev

Pending

User tests: Successful: Unsuccessful:

avatar shagungarg0
shagungarg0
9 Feb 2022

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

avatar shagungarg0 shagungarg0 - open - 9 Feb 2022
avatar shagungarg0 shagungarg0 - change - 9 Feb 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 9 Feb 2022
Category Front End Plugins
avatar zero-24
zero-24 - comment - 9 Feb 2022

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?

avatar brianteeman
brianteeman - comment - 9 Feb 2022

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


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36986.
avatar brianteeman brianteeman - test_item - 9 Feb 2022 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 9 Feb 2022

I have tested this item 🔴 unsuccessfully on 30a92b9


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

avatar brianteeman
brianteeman - comment - 9 Feb 2022

another valid email addresses that now fail is brian+text@example.com

original regex ==> brian+text@example.com ✔️✔️
This regex ==> text@example.com

avatar shagungarg0
shagungarg0 - comment - 10 Feb 2022
$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]

avatar shagungarg0 shagungarg0 - change - 10 Feb 2022
Labels Added: PR-3.10-dev
avatar richard67
richard67 - comment - 10 Feb 2022
$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.

avatar shagungarg0
shagungarg0 - comment - 10 Feb 2022
$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?

avatar richard67
richard67 - comment - 10 Feb 2022

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?

avatar shagungarg0
shagungarg0 - comment - 10 Feb 2022

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

avatar brianteeman
brianteeman - comment - 10 Feb 2022

345 is the length of the full email not the TLD

avatar shagungarg0
shagungarg0 - comment - 10 Feb 2022

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.

avatar richard67
richard67 - comment - 10 Feb 2022

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.

avatar shagungarg0
shagungarg0 - comment - 10 Feb 2022

Okk sir , i will take care of it and thank you for guidance.

avatar brianteeman brianteeman - test_item - 10 Feb 2022 - Tested successfully
avatar brianteeman
brianteeman - comment - 10 Feb 2022

I have tested this item successfully on ddf3975


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

avatar ChristineWk ChristineWk - test_item - 10 Feb 2022 - Tested successfully
avatar ChristineWk
ChristineWk - comment - 10 Feb 2022

I have tested this item successfully on ddf3975


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

avatar Quy Quy - change - 10 Feb 2022
Status Pending Ready to Commit
avatar Quy
Quy - comment - 10 Feb 2022

RTC


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

avatar zero-24 zero-24 - change - 20 Feb 2022
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: RTC
avatar zero-24 zero-24 - close - 20 Feb 2022
avatar zero-24 zero-24 - merge - 20 Feb 2022
avatar zero-24
zero-24 - comment - 20 Feb 2022

Merging thanks @shagungarg0 and the testers🎉

Add a Comment

Login with GitHub to post a comment