CRM-20969 Fix issues in order by clauses of reports showing errors in 7.1 #10765
Conversation
seamuslee001
commented
Jul 25, 2017
•
seamuslee001
commented
- CRM-20969: Fix issue in reports where we try and append a new array key to string
|
ping @monishdeb @eileenmcnaughton does this make sense to both of you? |
|
@seamuslee001 - alternative proposal #10779 |
seamuslee001
force-pushed
the
report_fixes
branch
from
1a7d919 to
11f3e49
Compare
| @@ -227,11 +227,11 @@ public function orderBy() { | |||
| foreach ($this->_columns as $tableName => $table) { | |||
| if (array_key_exists('order_bys', $table)) { | |||
| foreach ($table['order_bys'] as $fieldName => $field) { | |||
| $this->_orderBy[] = $field['dbAlias']; | |||
There was a problem hiding this comment.
It should be possible to simply remove this function
There was a problem hiding this comment.
I don't want to try that in this PR as i think that is a separate issue to deal with, This is just about making sure these reports pass php7.1
| @@ -268,25 +268,25 @@ public function groupBy() { | |||
| )) { | |||
| $append = ''; | |||
| } | |||
| $this->_groupBy[] = "$append {$this->_params['group_bys_freq'][$fieldName]}({$field['dbAlias']})"; | |||
There was a problem hiding this comment.
hopefully this whole function can go too
There was a problem hiding this comment.
Hmm - If I'm going to test out these reports I'd rather test the 'right' change. I just tested the Walklist report - with the existing (non 7.1 compliant) order by function sort by ascending works, sort by descending does not work, section break does not work. Removing the order by function causes it to work correctly (and fixes your php problem) - note the walklist report needs to be enabled under manage templates.
I am proposing this as an alternate approach for fixing the 7.1 errors. Note this only covers one report - I think we should move that function which is actually generic to the parent class, however I think the one in contribute.summary is a bit more nuanced. Note this is also the approach taken by the extendedreport extension (which takes it a little further). I will remove the declaration of groupByArray from that extension if we add it here I do have a medium term plan to move a lot of the helpers from extended report back into core. Change-Id: I377d33a7417ff93169f12ccbda77f3662da5bac1
seamuslee001
force-pushed
the
report_fixes
branch
from
11f3e49 to
47c0ee0
Compare
|
@eileenmcnaughton i have removed the orderBy Function given your testing, The groupBy ones seem to be more indivualised to the reports so i think they should be held onto, Only the walklist had issues with orderBy the others had issues with groupBy |
|
Actually they are not so much individualised as copy & pasted at different stages of development :-). If we overwrote the parent groupBy with the one from pledgeSummary we could remove it from most reports. However, I think we have met the requirement of including some code improvement in this PR so I'm OK to merge now |
|
Merging as per tag |
