[REF] Fix strstr deprecation in CustomDataByType class

[braders]

Overview

Fix strstr deprecation in CustomDataByType class.

Before

The following error could be logged:

[28-Dec-2022 19:55:40 Europe/London] PHP Deprecated:  strstr(): Passing null to parameter #1 ($haystack) of type string is deprecated in .../civicrm/CRM/Custom/Form/CustomDataByType.php on line 51

After

Ensure value passed to strstr is an empty string rather than null, avoiding deprecated functionality.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

I don't know what the circumstance is but does this actually work? Won't you have the same problem with trim() on the next line? Maybe up at line 36 would be better?

[demeritcowboy]

Does it even make sense that a request variable would contain CRM_Core_DAO::VALUE_SEPARATOR? Or that it could ever be an array when it specifies String in the retrieve()? I'm wondering if these lines are left over from some older version?

[braders]

I don't know what the circumstance is but does this actually work? Won't you have the same problem with trim() on the next line? Maybe up at line 36 would be better?

strstr('', CRM_Core_DAO::VALUE_SEPARATOR) will always return false, so we don't need to worry about the same issue occuring on line 52 (as it'll never be reached if $this->_subType is null).

That said, I'm equally happy to move to line 36, which also ensures we're passing a string to setGroupTree.

Does it even make sense that a request variable would contain CRM_Core_DAO::VALUE_SEPARATOR? Or that it could ever be an array when it specifies String in the retrieve()? I'm wondering if these lines are left over from some older version?

It's a very valid question. I've been doing some playing and can't find a case where $this->_subType would be a string seperated by CRM_Core_DAO::VALUE_SEPARATOR. That said, I couldn't say with certainty that I've tested every route, and it's also possible that extensions are calling civicrm/custom with a CRM_Core_DAO::VALUE_SEPARATOR subType param.

So I guess the question is how comfortable you are that this could be removed? Another option would be to add a call to CRM_Core_Error::deprecatedWarning within the if statement, specifying that the subType parameter should be comma-seperated.

[demeritcowboy]

Good thinking. I like the deprecation notice.
I note it was added at https://github.com/civicrm/civicrm-core/pull/6011/files, and it appears to be copy/paste from CRM/Custom/Form/CustomData.php.
Also noting that in the template it's used as

if (subType) {
  dataUrl += '&subType=' + subType;
}

so is really unlikely to be an array.

So I'm pretty confident that deprecating is valid.

[demeritcowboy]

Looks like I merged an earlier one too quickly without seeing the same thing although it's slightly different since the param can come from anywhere, but has the same trim() issue on the next line. The downside of PRs that have 18 disconnected things in them...