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

Some php warnings on case dashboard #20711

Merged
merged 1 commit into from
Jun 28, 2021

Conversation

demeritcowboy
Copy link
Contributor

Overview

    Notice: Undefined index: Resolved in include() (line 71 of .../sites/default/files/civicrm/templates_c/en_US/%%13/137/137BFF47%%DashBoard.tpl.php).
    Notice: Undefined index: Urgent in include() (line 71 of .../sites/default/files/civicrm/templates_c/en_US/%%13/137/137BFF47%%DashBoard.tpl.php).
    Notice: Undefined index: upcoming in include() (line 25 of .../sites/default/files/civicrm/templates_c/en_US/%%26/26F/26FE50E8%%CaseFilter.tpl.php).
    Notice: Undefined index: upcoming in include() (line 25 of .../sites/default/files/civicrm/templates_c/en_US/%%26/26F/26FE50E8%%CaseFilter.tpl.php).

Before

  1. Create at least one case.
  2. Turn on debugging under Admin - System Settings.
  3. Visit case dashboard.

After

Technical Details

For the case filter one, that form is re-used elsewhere where it has a checkbox called "upcoming", but on the case dashboard there is no such checkbox.

Comments

@civibot
Copy link

civibot bot commented Jun 25, 2021

(Standard links)

@civibot civibot bot added the master label Jun 25, 2021
@colemanw
Copy link
Member

Out of curiosity why isset instead of !empty? The latter is more like what was already there.

@demeritcowboy
Copy link
Contributor Author

demeritcowboy commented Jun 26, 2021

I waffled a bit myself. Here's a longer answer than you probably wanted and goes beyond this PR but maybe worth spinning off into a ticket.

In the case where the expected data type is an array and there's no loop on it, like here, I didn't see it making too much difference, and I figured that isset is more likely to surface programming errors, like if somehow it got assigned the value '0', albeit unlikely.

In the case where the expected data type is string, what you often want is neither isset nor empty, you want something like:

  • treat '' as false
  • treat false as an error (because you're expecting a string)
  • treat '0' as true, because while it might be unlikely depending on context, it's a valid string
  • treat 0 as an error
  • treat null as false
  • treat missing as ... it depends. Error can be better since it will catch typos, but places more of a burden on making sure it's always set in all callers.

And I admit in the other PR I used empty when it's really the above situation, but I figured '0' as a client name was super-unlikely, even in the future when we're replaced by robots and contact lists in CiviCRM are robot names.

@eileenmcnaughton
Copy link
Contributor

As an aside my preferred approach where possible is to ensure it is always assigned - not gonna hold this PR up on the bigger discussion

@eileenmcnaughton eileenmcnaughton merged commit 6152728 into civicrm:master Jun 28, 2021
@demeritcowboy demeritcowboy deleted the dashboard-err branch June 28, 2021 13:19
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.

3 participants