[PHP8] Fix warnings on activity and event management forms

[demeritcowboy]

Overview

Before

Go to New Activity. Screen fills with multiple lines of

Warning: Undefined array key "params" in include() (line 5 of .../templates_c/en_US/%%DF/DF0/DF0B0562%%RecurringEntity.hlp.php).
Warning: Trying to access array offset on value of type null in include() (line 5 of .../templates_c/en_US/%%DF/DF0/DF0B0562%%RecurringEntity.hlp.php).
Deprecated function: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in smarty_modifier_lower() (line 23 of .../packages/Smarty/plugins/modifier.lower.php).

After

No warnings. If you expand the repeat activity section the help bubbles still work.

Technical Details

The details are in the code comment, but briefly the file is parsed twice in the flow, the first time to get the help title, at which time the variable is not assigned anything.

Comments

This may overlap a little with #26265 but in the meantime this removes the warnings.

[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).

[larssandergreen]

More silly stuff because of that weird help title thing.

I guess the long term solution might be to switch to specifying the title in the tpl instead of the hlp (if different than the default name), since that's where we actually want it, and then we could stop evaluating the hlp just to get the titles. We could automate that conversion, but then there are extension to consider as well. But that's just some thoughts from wrapping my head around this.

I've reviewed and this makes sense and no issues on r-run. Works for now!

[demeritcowboy]

Thanks @larssandergreen. The other PR was marked draft but I see you've undrafted it. I'll take a look at that and then while setting this assign here would still be useful elsewhere, this tpl could then piggy back the other PR and be changed so it uses field labels.

And actually this particular tpl, for the second part of the warnings, I notice now it isn't translation friendly because of noun genders and also vowel first letters. For example the internet tells me activity is feminine in french, but since the translator doesn't know what %1 will be they have put un. Maybe worth removing the dynamic wording and using a fixed word like "record".

[larssandergreen]

@demeritcowboy That'd be great.

I think the translations aren't too bad (most of them are your so votre either way in French). Probably not worth them all being re-translated.

[demeritcowboy]

I've rebased but in thinking about the .extra.hlp files, I probably shouldn't change the ids because then existing extra.hlp files will stop working. So this PR is still the same as before.

[demeritcowboy]

jenkins retest this please

[demeritcowboy]

Putting merge-ready based on earlier review.

[totten]

@demeritcowboy... This may a bit marginal, but...

1. {help} appears to be a utility that's sprinkled into many different contexts. For example, templates/CRM/Contact/Form/Merge.tpl.
2. The name $params is common in Civi. For example, Merge.tpl has its own usage of $params (with different data).

I don't know if this is actually a bug right now, but you probably only want to assign params temporarily (while processing {help}) rather than leaving $params==[] as a side-effect.

Perhaps something like this:

    $smarty->pushScope(['params' => []]);
    $popScope = CRM_Utils_AutoClean::with([$smarty, 'popScope']);

[totten]

Or equivalently, change the idiom from

$smarty->assign(VAR, VALUE);
$smarty->fetch(TEMPLATE);

to

$smarty->fetchWith(TEMPLATE, [
  VAR => VALUE
]);

(fetchWith() calls fetch() and temporarily assigns variables.)

[demeritcowboy]

Ah I like that better, since as far as I can see popScope would still leave params at [] if there was no params before, which is often going to be the case.

And just to be extra careful, I would call fetchWith with whatever current template_vars, plus params if it's not already assigned.

Thanks.

[demeritcowboy]

Oh fetchWith just does push and pop scope so it's the same thing. What we really want here is:
if params is missing, set it, then UNSET it after (only if it was missing before).
if params is there, then all good.

[totten]

Oof, I assumed that popScope() would unset, but I think you're right - in the case where we'd want unset(), it would probably leave []. It's still better with this slightly-buggy cleanup ($params will be restored if it's substantive...). But yeah, we should take a stab at fixing that too...

[totten]

Nice. Approximately 30 million warnings went away from that screen.

[demeritcowboy]

Thanks!