-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Throw spaghetti #22070
Throw spaghetti #22070
Conversation
(Standard links)
|
7fbf210
to
da1365d
Compare
243ea6c
to
4a3ea78
Compare
4a3ea78
to
96b1435
Compare
96b1435
to
27308df
Compare
27308df
to
6e1fbb9
Compare
test this please |
@seamuslee001 the spaghetti stuck! |
Neat. Can you maybe add a description to this PR @eileenmcnaughton so we know what we're looking at (other than Tim's spaghetti monster illustration) |
@colemanw doesn't it say it all? |
Ok, tests are happy and this is a step in the right direction. |
I've hit a side-effect of the addition of 6 lines to CRM/Report/Form.php at line 2500:
In a custom report, with the above change in place, The custom report does all its business in postProcess(), with a fixed set of columns defined there with e.g.
Then constructs a query & runs buildRows(), formatDisplay(), doTemplateAssignment(), endPostProcess(). Worked in 5.35.2. Does this need rejigging to work with current CRM/Report/Form.php or should CRM/Report/Form.php be amended? |
@davejenx so my reading is that the lines above unset no_display and then they are re-added but the 'ensure fields are set part' - it feels like the easiest would be to reverse the order- so the isset happens last? |
Hi @eileenmcnaughton , thanks for taking a look! Yes, reversing the order works, so that the 6 added lines (from Or as I suggested, put those 6 lines within an |
@davejenx do you want to put one of those options up as a PR |
Thanks @eileenmcnaughton , done: #23321 . |
Overview
Enable escape on output for unit tests
Before
Not enabled for unit tests - issets & things that are notices when it is on sneak through
After
Enabled for unit tests - although I suspect only some since I think there would have been more failures otherwise.
Technical Details
Comments
https://github.com/civicrm/civicrm-core/pull/22070/files#diff-b0e39754f5c0209c45b75774d772d429b3f947203d0681794120256f1c4d2c68R412 is the key line