CRM-19811 fix one instance of referring to LOWER() & comment others. #9596
Conversation
I have tested & removed the current_employer reference & added notes to other references to LOWER. I will track down & remove the one relating to credit notes in the next round
|
I have only changed the 2 instances of LOWER for which I have written unit tests and commented all the rest. I am pretty confident in my analysis that they can all go but have limited myself to commenting the others for now so they can be removed later |
| $query->_where[$grouping][] = CRM_Contact_BAO_Query::buildClause($wc, $op, $value, $dataType); | ||
| $query->_qill[$grouping][] = "$whereTable[title] $op $quoteValue"; | ||
| list($tableName, $fieldName) = explode('.', $whereTable['where'], 2); | ||
| list($tableName) = explode('.', $whereTable['where'], 2); |
There was a problem hiding this comment.
$fieldName is not used anywhere - minor tidy up
eileenmcnaughton
force-pushed
the
performance
branch
from
0d6d95c to
548925b
Compare
Also fix historical sql to guard against copy & paste
…s (e.g. creditnote_id). Add unit test to demonstrate case sensitivity still respected
eileenmcnaughton
force-pushed
the
performance
branch
from
548925b to
2b3e31a
Compare
|
I can offer some quid-pro-quo review on this if anyone wants to review it |
|
this looks sound to me (for what that is worth!) - is there some kind of check in the installation code or admin panel that verifies that mysql is configured to case-insensitive text match? something like |
|
Well the installation code specifies the table creation & in that it specifics the Collation - any collation ending in _ci is case insensitive. So, the only way I think someone could be other than case insensitive is if they have deliberately changed their Collation from the default - and I don't think we have to be responsible for every possible hack someone may have done |
|
(ie. collation is set per table / field) |
|
oh. right. well that takes care of the only gotcha I could think of :-) only other thought was is there a convention for tagging comments with |
|
We follow the drupal coding standards for those things https://www.drupal.org/docs/develop/coding-standards/api-documentation-and-comment-standards#todo |
|
cool - no fixme then. worth a todo? (btw, I'm still wrestling with getting my proper tools operational but hopefully I'll be in a position to help expand unit test coverage soonish) |
|
@eileenmcnaughton I'll take you up on your quid-pro-quo offer... how about this one while we're on the topic of efficiency? #9623 |
I have tested & removed the current_employer reference & added notes to other references to LOWER. I will track down & remove the one relating to credit notes in the next round