Fix for dev/core#428: Fatal error in Activity Details report when Sorting uses Section Header

[twomice]

Overview

This PR corrects a bug in the Activity Details report, in which a fatal error is caused by setting "Section header" for a column under the "Sort" tab.

Before

In the Activity Details report, a fatal error is caused by setting "Section header" for a column under the "Sort" tab.

After

No more fatal error.

Technical Details

Currently, the activity Details report assumes the name of a temporary table is stored in $this->temporaryTables['activity_temp_table'], but that's really an array. The table name is actually stored in $this->temporaryTables['activity_temp_table']['name']. This PR fixes that.

Comments

(none)

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[ErikHommel]

@twomice as far as I can see the tests failed? Is that correct?

[twomice]

@ErikHommel Those 7 tests are failing with all the PRs I've reviewed recently. They're also testing things that seem very far removed from the scope of this change, so I'm virtually positive they're unrelated to this PR.

[ErikHommel]

@twomice Cool, I have already asked for a reviewer....:-)

[eileenmcnaughton]

• (r-explain) PASS
• (r-test) PASS
• (r-code) PASS
• (r-doc) PASS
• (r-maint) PASS
• (r-run) PASS:
• (r-user) PASS:
• (r-tech) PASS_:

@twomice can you put this against the rc as a recent regression

[MarshCastle]

I just tried this patch on a civibuild drupal-demo, but although the patch does fix the issue it seems to have created an error on every page load:

Strict warning: Declaration of CRM_Volunteer_Permission::check() should be compatible with CRM_Core_Permission::check($permissions, $contactId = NULL) in CRM_Core_ClassLoader::loadClass() (line 218 of /home/marsh/bknix/build/drupal-demo/sites/all/modules/civicrm/CRM/Core/ClassLoader.php).

I'll try again just to be sure...

[MarshCastle]

Yep - confirmed:

• removed the patch, and the functionality is broken
• replace the patch and the issue is fixed, but there's an error on the Activity Report page (/civicrm/report/instance/3)

[twomice]

Jenkins test this please.

[twomice]

Hi @eileenmcnaughton I'm not sure what you mean by "put this against the rc as a recent regression". Can you restate?

[eileenmcnaughton]

@twomice can you rebase the patch over the 5.7 branch and change the pr to be against the 5.7 branch. That way it will be in the November release - which makes sense as it is a fix for a recent regression. (Optionally we could drop a 5.6.x release but we'll make that call once it's merged to 5.6 & - we take into account things like how far through the the month we are, how old the regression is & whether there are other things to go out)

[twomice]

@eileenmcnaughton I think this is where i always get tripped up. Is this right?

[eileenmcnaughton]

@twomice it's a little bit right & a lot wrong. 2 ways to fix it
1)git rebase -i origin/5.7
(& delete all the patches except this one)
2) git reset --hard origin/5.7
followed by a git cherry-pick of your commit

either way you need to do a git push -f to push it up afterwards

hint
git reflog
will help you find a commit if you lose the commit hash doing git reset --hard (which resets you git pointer & your files to the commit you point to.

[twomice]

Thanks @eileenmcnaughton ! Done. Please prompt me if it needs something more.

[eileenmcnaughton]

thanks @twomice ! merging