dev/core#2769 use php email validation not hacked & bad quickform function #21169
Conversation
|
(Standard links)
|
eileenmcnaughton
changed the title
eileenmcnaughton
force-pushed
the
email
branch
from
6792e35
to
6dc9a15
Compare
|
@seamuslee001 that was a good call you made about requiring testing of the email address - I added the original one to the test & it does fail (this will) - I think the answer is to make our Utils_Rule function handle it (whatever it takes) - since it will be much easy to maintain a tested CRM function going forwards than a hacked QF fuction |
eileenmcnaughton
force-pushed
the
email
branch
from
6dc9a15
to
54364b2
Compare
|
@seamuslee001 you might like to re-check this as I've made changes - I've copied over the idn_to_ascii from the hack to our core code & modified it so that it works with both variants I've also carved out an exception for 'test@localhost' for sites in developer mode. Since @aydun raised it I thought we might as well handle it |
seamuslee001
added
merge ready
|
This seems to make sense to me changed to merge ready in case anyone else wants to chime in |
|
Looks good to me as well. |
CRM/Utils/Rule.php
Outdated
| foreach ($parts as &$part) { | ||
| // if the function returns FALSE then let filter_var have at it. | ||
| $part = self::idnToAsci($part) ?: $part; | ||
| if ($part === 'localhost' && Civi::settings()->get('environment') === 'Development') { |
There was a problem hiding this comment.
Is there a need to check for Civi::settings()->get('environment') === 'Development'? Buildkit doesn't set this, the PR test runs and test sites don't set it, I never set it, and the original purpose of that setting I think had to do with stopping cron jobs from accidentally sending things out.
There was a problem hiding this comment.
@demeritcowboy dunno - I can remove if people think I should - anyone else have an opinion?
There was a problem hiding this comment.
Emoji poll over on chat https://chat.civicrm.org/civicrm/pl/mckup3magtnid853ysaj5qga3h
There was a problem hiding this comment.
And the unicorns have it - the environment check is gone
There was a problem hiding this comment.
Thanks @eileenmcnaughton
eileenmcnaughton
force-pushed
the
email
branch
from
54364b2
to
3bff011
Compare
tests/phpunit/CRM/Utils/RuleTest.php
Outdated
| * Test that 'test@localhost' works in dev environments. | ||
| */ | ||
| public function testDevelopmentEmail(): void { | ||
| Civi::settings()->set('environment', 'Development'); |
There was a problem hiding this comment.
@eileenmcnaughton This line needs to go too?
There was a problem hiding this comment.
@mattwire yeah - I just removed that test as it is meaningless now
Per https://lab.civicrm.org/dev/core/-/issues/2769 we have had problems over the years with quickform's email validation and we now have a hacked version that is problematic from a maintenance pov & also doesn't work with the string I have just encountered: name.-o-.i.10@example.com (which I am told is valid and which passes the php filter). We already have an email rule which calls a php native function which is better maintained than our layers of hacks. This PR registers our email rule - which overrides the quickform one. If we merge this we can revert quickform back to unhacked which will improve debugging and maintenance (although it's actually bypassed now with this change)
eileenmcnaughton
force-pushed
the
email
branch
from
3bff011
to
5ae4628
Compare
|
I think we can merge this now? |
|
Looks good to me |
|
thanks all - I think having less hacked spaces will be better for our brains @seamuslee001 did you see the related package prs to remove the QF hack |
Overview
dev/core#2769 use php email validation not hacked qf
Before
Cannot save email
name.-o-.i.10@example.comthrough back office contact edit screen (& others based on where this rule is in place)After
It saves, but an obviously wrong email doesn't
Technical Details
Per https://lab.civicrm.org/dev/core/-/issues/2769 we have had problems over the years with
quickform's email validation and we now have a hacked version that is
problematic from a maintenance pov & also doesn't work
with the string I have just encountered: name.-o-.i.10@example.com
(which I am told is valid and which passes the php filter).
We already have an email rule which calls a php native function
which is better maintained than our layers of hacks. This
PR registers our email rule - which overrides the quickform
one. If we merge this we can revert quickform back to
unhacked which will improve debugging
and maintenance (although it's actually bypassed
now with this change)
Comments