Skip to content

Document best practices for how to handle E_NOTICES/WARNINGS

There's been a lot of PRs the last year that fix php notices but doing it in new and wonderful ways. There's https://docs.civicrm.org/dev/en/latest/security/outputs/#escape-by-default and the general issue in dev/core#1328 (closed) but feels like it needs more guidelines.

It's pretty draft right now and just looking for some place to put it so dumping in this ticket.

In smarty templates:

  • isset() doesn't work with CIVICRM_SMARTY_DEFAULT_ESCAPE because the target gets converted to an expression which isn't allowed with isset(). You can see this in regular php or with escape turned off if you do something like {if isset(is_array($foo) ? $foo.bar : $foo)}
  • empty() doesn't work with CIVICRM_SMARTY_DEFAULT_ESCAPE because the target gets evaluated by smarty/php before being sent to empty(), so you still get the error.
  • array_key_exists works, but:
    • Only works on arrays (duh)
    • Has the same limitation as empty() because the expressions involved will be evaluated first, e.g. {if array_key_exists('bar', $form.foo)} will still give the error if the foo element of $form does not exist.

But even before all that, what needs to be considered is what the error is really about and the difference between the variations when $x is a falsey value. For example:

  • You don't want to silence when it's telling you that the variable name is a typo. This is trickier when it's a variable variable, like $form.$field.
  • Or when you're expecting it to be there, e.g. a contact id on an existing contribution. Why is it missing? Maybe the real problem is a typo along the way or some bad logic.
  • What is the code actually doing? Is it even needed or is it left over from older code and that is why there is an error? Can it be removed?
  • A common variable is $form.foo which represents a quickform field, and it will have two subkeys $form.foo.label and $form.foo.html. Suppose it's the type of field that legitimately may not be present, like a money-related field on a free event. Both {if $form.foo} and {if $form.foo.html} would give errors in this situation. Note we can't use {if array_key_exists('html', $form.foo)} because it will still give the error for as mentioned earlier, but in this case {if array_key_exists('foo', $form)} is sufficient if the check was about whether the field exists or not.
  • But note there is a big difference for $form.foo if it's something like a boolean field, e.g. between {if array_key_exists('showTitle', $form)} and {if $form.showTitle} when showTitle is 0 or FALSE or NULL. You'll end up with titles showing when they shouldn't. In this case you want to make sure showTitle is always assigned explicitly to something in the php, and then use {if $form.showTitle} in the template.

In php

(TODO: flesh out this part)

Similar considerations apply on the php end. Using ?? is not always the right choice.

FYI @eileen @bradleyt

Edited by DaveD