dev/core#1108 [REF] use CRM_Core_DAO::executeQuery instead of ->query() #14760
Conversation
This allows us to leave the multilingual translation to the I18n rewriter. This PR just removes 2 instances from this function. Once the others are removed we should be able to remove the swath of code defining I18n tables are the start of the function and simplify the tables involved Note testUnsubscribeGroupList passes through this code
|
(Standard links)
|
|
looks safe to merge |
| SELECT $mg.entity_table as entity_table, | ||
| $mg.entity_id as entity_id | ||
| FROM $mg | ||
| FROM civicrm_mailing_group $mg |
There was a problem hiding this comment.
@eileenmcnaughton this looks off to me, why are you specifiying the table name here?
There was a problem hiding this comment.
@seamuslee001 so as I understand it & @mlutfy can jump in if I am wrong we don't need to translate the table name - we just call CRM_Core_DAO::executeQuery and by default that has I10n handling on and it deals with it.
The reason THIS code has had to do all this mucking around is because it didn't want to conform & wanted to show independence by calling a retro function that doesn't handle I10n - but sometimes you just don't get to be an individual
There was a problem hiding this comment.
I think the other thing is what about when your coming via an extern URL? Like from the {mailing.opt_out_url} token. In those cases have we bootstrapped in all the CRM classes or just done require once?
There was a problem hiding this comment.
ah - good thinking - we should test!
| SELECT $group.id as group_id, | ||
| $group.title as title, | ||
| $group.description as description | ||
| FROM $group | ||
| LEFT JOIN $gc | ||
| FROM civicrm_group $group |
There was a problem hiding this comment.
Again why are we specifiying the table name here rather than in $group?
There was a problem hiding this comment.
@eileenmcnaughton can you comment on this please?
There was a problem hiding this comment.
right so my understanding . is it was because we weren't relying on l10n in the query object - but @seamuslee001 believes it might not be available via extern path
|
@seamuslee001 I did a search for this code & I can't see any evidence it's coming from the extern directory or any unbootstrapped paths.
I stepped through the code by calling it from the api
And you can see the query IS translated - ie the alias here is already translated so it goes from to FROM civicrm_group_US civicrm_group_US
SO I feel like swapping out the dao->query calls fro the std executeQuery IS the right way to stdise these (@mlutfy ) |
|
I'm not familiar enough with the Regarding external paths, the unsubscribe page does fully bootstrap, no? (ex: it works in non-English) I will deploy the patch to our production sites, so hopefully if this causes any regressions, we will find out before this goes to the next stable. Until then, I'm in favour of merging. |
|
given @mlutfy 's comments i'm happy to merge this now and we will see what happens |
|
thanks @seamuslee001 |
Overview
Minor code cleanup
Before
Query translation done ad hoc in the function
After
Query translation done in standardised way by executeQuery
Technical Details
Fixes 2/4 places where this pattern is in the function. Fixing the last 2 will reduce cruft quite a bit
Comments