REF Remove some duplication in CRM_Utils_Type::escape/validate #14577
Conversation
|
(Standard links)
|
|
@mattwire test failures look like they relate |
mattwire
force-pushed
the
type_consolidation
branch
2 times, most recently
from
9d656c3 to
38aaf83
Compare
mattwire
force-pushed
the
type_consolidation
branch
from
38aaf83 to
ab2fa56
Compare
|
@seamuslee001 @colemanw This one is now ok for review / test if you have time |
| } | ||
|
|
||
| if (preg_match('/^\d{8}$/', $data) && | ||
| CRM_Utils_Rule::mysqlDate($data) |
There was a problem hiding this comment.
Isn't this is a functional difference, since 'Date' only accepted date input, and Timestamp accepts Date+Time?
There was a problem hiding this comment.
@mlutfy Yes you are right. Looking in core it seems the validate(xx, 'Date') is not really used, and extensions have been using "Integer" to validate DateTime. https://github.com/eileenmcnaughton/nz.co.fuzion.extendedreport/blob/master/CRM/Extendedreport/Form/Report/Campaign/CampaignProgressReport.php#L144
For escape we accept both. So this change would allow Date to validate a DateTime as well which I think it should have been doing anyway? @seamuslee001 @eileenmcnaughton @pfigel What are your thoughts on this point?
There was a problem hiding this comment.
@mattwire I agree that being consistent in validate and escape would make sense.
I found some usages of CRM_Utils_Type::validate() with $type='Date' in universe, e.g. in uk.org.futurefirst.networks.civipoints/CRM/Points/Page/AJAX.php. I don't believe the change in behaviour would really cause a serious issue in this case (worst case I imagine MySQL would truncate the time portion while potentially emitting a warning). There should at least be no security impact.
There was a problem hiding this comment.
Overall I feel like this make sense & makes the code less complex (which is always a plus for security-sensitive bits). There's good existing test coverage and I haven't found any obvious issues during light r-run. There's some potential for breakage for extensions that relied on the difference in behaviour in validate() and escape for date values, but IMO being consistent should be the priority here.
|
Merging based on @pfigel review |
Overview
CRM_Utils_Rule::escape()andCRM_Utils_Rule::validate()duplicate the same checks.Before
CRM_Utils_Rule::escape()andCRM_Utils_Rule::validate()duplicate the same checks.After
CRM_Utils_Rule::escape()andCRM_Utils_Rule::validate()share the same checks.Technical Details
This will change the wording of the fatal error messages (for the better) for some failed type checks.
Comments