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

Fix undefined tpl vars in CiviReport #20797

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 7, 2021

Overview

Fixes a whole bunch of PHP notices on CiviReport screens.

@civibot
Copy link

civibot bot commented Jul 7, 2021

(Standard links)

@civibot civibot bot added the master label Jul 7, 2021
@demeritcowboy
Copy link
Contributor

FYI some test fails:

api_v3_JobTest.testMailReportForPrint
CRM_Report_Utils_ReportTest.testOutputPrint

@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

Yea I left off an ! from an empty(). Glad the tests caught it.

I'm toying with the idea of using a mass regex replace to do simple transformations like {if $foo.bar} => {if !empty($foo.bar)}. It would result in a daunting PR but IMO is safer because a human doesn't have to write it.
It wouldn't be able to do the weird cases but could save us a lot of manual work for the simple ones.

@demeritcowboy
Copy link
Contributor

I kind of agree with @eileenmcnaughton that some of these are pointing out places where something is missing that shouldn't be, maybe even some rogue typos, and so a mass replace with empty would hide those, but on the other hand it would be no different than it's been for years. So 🤷

@demeritcowboy
Copy link
Contributor

jenkins retest this please

E2E\Core\AssetBuilderTest::testInvalid

@eileenmcnaughton eileenmcnaughton merged commit 392c3dc into civicrm:master Jul 7, 2021
@eileenmcnaughton eileenmcnaughton deleted the reportTplFixes branch July 7, 2021 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants