Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use array_key_exists to avoid warning on report GroupBy.tpl template #24631

Merged

Conversation

braders
Copy link
Contributor

@braders braders commented Sep 27, 2022

Overview

Use array_key_exists to avoid warning on report GroupBy.tpl template.

Before

PHP warnings (notice up until PHP8) were thrown whenever a report with group options was loaded. Undefined array key. As the warnings were happening in a loop this could cause a lot of noise when viewing error logs.

After

This warning is gone (lots still remaining on reports - baby steps)

Technical Details

Tested with and without CIVICRM_SMARTY_DEFAULT_ESCAPE, and on a handful of reports.

@civibot
Copy link

civibot bot commented Sep 27, 2022

(Standard links)

@civibot civibot bot added the master label Sep 27, 2022
@demeritcowboy
Copy link
Contributor

This didn't fully work for me and that kind of makes sense to me. If the higher-level element $form.group_bys_freq doesn't exist then you'll still get an error (repeat contributions report is an example).

Could have two ifs, or an && seems to work. Or back to ensuring it exists in php.

@braders
Copy link
Contributor Author

braders commented Sep 27, 2022

This didn't fully work for me and that kind of makes sense to me. If the higher-level element $form.group_bys_freq doesn't exist then you'll still get an error (repeat contributions report is an example).

Could have two ifs, or an && seems to work. Or back to ensuring it exists in php.

Good catch, I had not tested a report that had grouping but no group_bys_freq. I think making sure group_bys_freq array always exists is the right approach. It's a bit late now but I'll look at fixing in the next day or so.

@braders braders marked this pull request as draft September 27, 2022 21:27
@braders braders force-pushed the report-groupby-array-key-exists branch from 6ffb3e2 to a3f6a93 Compare September 28, 2022 20:06
@braders braders marked this pull request as ready for review September 28, 2022 20:08
@braders
Copy link
Contributor Author

braders commented Sep 28, 2022

@demeritcowboy This should be working correctly now.

@demeritcowboy demeritcowboy merged commit f65dc83 into civicrm:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants