Update civireport.md

[tschuettler]

The values of those field parameters are currently not evaluated. It is only checked if the parameter exists at all.

Alternatively it would require a bit of patching to change the functionality to work with true and false values.

[seancolsen]

Can someone else with a bit more experience with CiviReport review this docs change? Perhaps @eileenmcnaughton @michaelmcandrew @MegaphoneJon

[MegaphoneJon]

I think that both "true/false" and "not empty" are ambiguous, and I would suggest "boolean" as the alternative.

[tschuettler]

@MegaphoneJon @seanmadsen The issue was probably not explained clear enough by myself.
Currently there is no check for the value itself, it is only tested if the parameter is used. The value is not used at all.

I stumbled across this when trying to change a required field to be not required by setting it to
'required' => FALSE, which would still set the form logic as beeing a required field due to checks like these:
https://github.com/civicrm/civicrm-core/blob/master/CRM/Report/Form.php#L750

[michaelmcandrew]

I think that tschuettler is saying that, for example, if you set the value of no_repeat to false, it will be repeated in any case. @MegaphoneJon, if that is the case, then boolean would be just as wrong as true/false.

In most cases, I think testing for the value of a variable (or constant) is better than testing to see if it is set.

Although it is a bit more work, I think in this case, it makes more sense to correct the behaviour in the code rather than document the bad behaviour. I can't imagine many people are relying on that behaviour and it is useful to be able to override stuff.

Failing the desire to fix the bug, I would accept this PR, but would encourage you to see how much work a fix would be.

[michaelmcandrew]

@tschuettler - looks like I was replying at the same time as you.

To get an idea of the scale of the problem, I had a look through the report directory.

Out of 175 lines with no_display in, 161 of these are of the form "no_display...TRUE".

My hunch is that, with an appropriate warning, it would be reasonable to break the existing bad behaviour.

buildkit@civicrm:~/build/dmaster/sites/all/modules/civicrm/CRM/Report/Form$ grep -Pr no_display | wc -l
175
buildkit@civicrm:~/build/dmaster/sites/all/modules/civicrm/CRM/Report/Form$ grep -Pr no_display.+TRUE | wc -l
161
buildkit@civicrm:~/build/dmaster/sites/all/modules/civicrm/CRM/Report/Form$ grep -Pr no_display.+FALSE | wc -l
0

[tschuettler]

When grepping universe there are some extensions not using TRUE for no_display or no_repeat. There is too much noise for me to analyse required.

/opt/buildkit/build/universe$ grep -nrP "'(no_display|no_repeat)'".+FALSE | wc -l
17

I also found a single [...]['no_display'] = 1; in:
https://github.com/eileenmcnaughton/nz.co.fuzion.extendedreport/blob/60239a272e88ee9487e8812dadf2b0030a2b14ab/CRM/Extendedreport/Form/Report/ExtendedReport.php#L512

I do agree that fixing would be better, but the workaround of not having the parameter is good enough for me.

[eileenmcnaughton]

So ideally we are encouraging people to set true / false even if we are handling any truthy value as true (pretty much to avoid e-notices).

[michaelmcandrew]

even if we are handling any truthy value as true

FWIW in this instance, we are handling a falsey value as true

[eileenmcnaughton]

If we are handling a falsey value as true then that is a bug - I assume you mean we are using isset instead of empty somewhere?

[michaelmcandrew]

Quoting @tschuettler from here

I stumbled across this when trying to change a required field to be not required by setting it to
'required' => FALSE, which would still set the form logic as beeing a required field due to checks like these:
https://github.com/civicrm/civicrm-core/blob/master/CRM/Report/Form.php#L750

if (!empty($field['no_display']))

The triple negative of ! + empty + no_display is making my brain hurt a little.

[eileenmcnaughton]

so if $field['no_display'] = FALSE then that is 'empty'

so that means if $field['no_display'] is set & is something other than

"" (an empty string)
0 (0 as an integer)
0.0 (0 as a float)
"0" (0 as a string)
NULL
FALSE
array() (an empty array)

http://php.net/manual/en/function.empty.php

So any falsey value should mean the field is not no_display - ie. the field IS display

[michaelmcandrew]

I am probably not giving this the attention it needs.

@tschuettler's says "I stumbled across this when trying to change a required field" but the example given was about no_display.

It appears that these are all handled in a slightly different way. It would be good to be sure that we have definitely nailed down the behaviour (seems like it is different in each case) before accepting the PR to the docs.

[tschuettler]

Sorry guys, i jumped to the wrong conclusion, that they would all be handled the same way.

I have opened a PR to fix the bug that a falsey value of required is handled as true: civicrm/civicrm-core#11725.

Other than that this would be just about not encouraging to use booleans. Which is not ideal even though we don't explicitly check for them.

I'm in favour of closing this one once the bug fix is merged.