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

Fix possibly unefined $form variables in smarty templates #21735

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Oct 5, 2021

Overview

Fixes a bunch of e-notices coming from smarty templates using mass regex find & replace.

Technical Details

Referencing a property of the $form in smarty is almost always going to be undefined if it hasn't been set on the form, so this fixes a bad pattern of not using empty() to guard against e-notices.

Referencing a property of the $form in smarty is almost always going to be undefined
if it hasn't been set on the form, so this fixes a bad pattern of not using empty()
to guard against e-notices.
@civibot
Copy link

civibot bot commented Oct 5, 2021

(Standard links)

@civibot civibot bot added the master label Oct 5, 2021
@demeritcowboy
Copy link
Contributor

  1. Argument for this is that it's no different than before.
  2. Argument against is that blanket using empty hides things like typos, or when it should actually be set by the corresponding .php but there's a logic flaw and it skips getting set. It also hides the situation where somebody later makes a change to the .php but forgets to update the .tpl.

But it's not always easy to say "it should always be set to something, even just null if need be". So 50/50.

I glanced over it and nothing jumped out at me here.

@colemanw
Copy link
Member Author

colemanw commented Oct 6, 2021

In general I agree that mass-updating templates with !empty() could mask some underlying problems. However in the case of accessing $form elements, they are created via CRM_Core_Form::addElement or addField methods rather than through $this->assign() so 9 times out of 10 if the template says {if $form.foo} it's a lazy/sloppy substitute for {if !empty($form.foo)}.

@eileenmcnaughton
Copy link
Contributor

Yeah - I'm OK with this - I'm getting a bit fed up with these notices & I agree that these are a bit less likely to be completely obsolete than assigns

@@ -94,7 +94,7 @@
</tfoot>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -29,7 +29,7 @@
<div class="clear"></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -14,7 +14,7 @@
</td>
</tr>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -22,7 +22,7 @@
<td class="label">{$form.icon.label} {help id="id-menu_icon" file="CRM/Admin/Form/Navigation.hlp"}</td>
<td>{$form.icon.html} </td>
</tr>
{if $form.parent_id.html}
{if !empty($form.parent_id.html)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -58,37 +58,37 @@
<tr class="crm-paymentProcessor-form-block-user_name">
<td class="label">{$form.user_name.label}</td><td>{$form.user_name.html} {help id=$ppTypeName|cat:'-live-user-name' title=$form.user_name.label}</td>
</tr>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -212,7 +212,7 @@
</table>
<div class="crm-submit-buttons">{include file="CRM/common/formButtons.tpl" location="bottom"}</div>
</div>
{if $form.contact_edit_options.html}
{if !empty($form.contact_edit_options.html)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -20,7 +20,7 @@
<tr class="crm-preferences-date-form-block-date_format">
<td class="label">{$form.date_format.label}</td><td>{$form.date_format.html}</td>
</tr>
{if $form.time_format.label}
{if !empty($form.time_format.label)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -19,7 +19,7 @@
<td class="label">{$form.lcMessages.label}</td>
<td>{$form.lcMessages.html}</td>
</tr>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -17,7 +17,7 @@
<td class="label">{$form.userFrameworkUsersTableName.label}</td>
<td>{$form.userFrameworkUsersTableName.html}</td>
</tr>
{if $form.wpBasePage}
{if !empty($form.wpBasePage)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -15,7 +15,7 @@
</td>
</tr>

<tr id="option_group" {if !$form.option_group_id}class="hiddenElement"{/if}>
<tr id="option_group" {if empty($form.option_group_id)}class="hiddenElement"{/if}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -233,7 +233,7 @@
</td>
</tr>
{/if}
{if $form.tag.html}
{if !empty($form.tag.html)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -45,7 +45,7 @@
{$form.activity_type_filter_id.html}
</td>
</tr>
{if $form.activity_deleted}
{if !empty($form.activity_deleted)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -53,7 +53,7 @@
</tr>
{/if}

{if $form.activity_details.html}
{if !empty($form.activity_details.html)}
<tr class="crm-case-form-block-activity_details">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -41,7 +41,7 @@
<br />
{$form.case_owner.html}
{/if}
{if $form.case_deleted}
{if !empty($form.case_deleted)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -10,7 +10,7 @@
{* tpl for building Individual related fields *}
<table class="form-layout-compressed">
<tr>
{if $form.prefix_id}
{if !empty($form.prefix_id)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@eileenmcnaughton
Copy link
Contributor

I can't keep myself focussed on checking this many changes - I've commented where I have checked so far

@@ -11,7 +11,7 @@
<fieldset class="crm-group crm_user-group">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -20,13 +20,13 @@
<td class="label">{$form.description.label}</td>
<td>{$form.description.html}</td>
</tr>
{if $form.parent_id.html}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -63,14 +63,14 @@
</td>
</tr>
{* fix for CRM-10241 *}
{if $form.count.html}
{if !empty($form.count.html)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -81,7 +81,7 @@
</tr>

{if $email and $outBound_option != 2}
{if $form.is_acknowledge }
{if !empty($form.is_acknowledge)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -19,7 +19,7 @@

<div id="pcpFields">
{crmRegion name="pcp-form-pcp-fields"}
{if $form.target_entity_type}
{if !empty($form.target_entity_type)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -37,7 +37,7 @@
</tr>
<tr>
<td>
{if $form.member_auto_renew}
{if !empty($form.member_auto_renew)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -12,7 +12,7 @@
<div class="help">
{ts}Use this form to enable and configure a Membership Signup and Renewal section for this Online Contribution Page. If you're not using this page for membership signup, leave the <strong>Enabled</strong> box un-checked..{/ts} {docURL page="user/membership/setup"}
</div>
{if $form.membership_type.html}
{if !empty($form.membership_type.html)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -26,7 +26,7 @@
{$form.is_archived.html}
</div>
</td>
{if $form.mailing_status}
{if !empty($form.mailing_status)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -35,7 +35,7 @@
<div class="clear"></div>
</div>

{if $form.contact_type}
{if !empty($form.contact_type)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -7,7 +7,7 @@
<td>{$form.buttons.html}</td>
</tr>
<tr>
{if $form.contact_tags}
{if !empty($form.contact_tags)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this file = ok

@@ -2,11 +2,11 @@
<table>
<tr>
<td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -72,13 +72,13 @@
<div>{$form.location_type.label} {help id="location_type" title=$form.location_type.label}</div>
{$form.location_type.html}
</div>
{if $form.address_name.html}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -3,7 +3,7 @@
<tr>
<td>{$form.operator.label} {help id="id-search-operator"}<br />{$form.operator.html}</td>
<td>
{if $form.deleted_contacts}{$form.deleted_contacts.html} {$form.deleted_contacts.label}{/if}
{if !empty($form.deleted_contacts)}{$form.deleted_contacts.html} {$form.deleted_contacts.label}{/if}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -33,7 +33,7 @@
<tr class="crm-contact-task-addtogroup-form-block-description">
<td class="label">{$form.description.label}</td>
<td>{$form.description.html}</td></tr>
{if $form.group_type}
{if !empty($form.group_type)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -26,14 +26,14 @@
{$form.to.html} {help id="id-to_email" file="CRM/Contact/Form/Task/Email.hlp"}
</td>
</tr>
<tr class="crm-contactEmail-form-block-cc_id" {if !$form.cc_id.value}style="display:none;"{/if}>
<tr class="crm-contactEmail-form-block-cc_id" {if empty($form.cc_id.value)}style="display:none;"{/if}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -8,7 +8,7 @@
+--------------------------------------------------------------------+
*}
{*common template for compose PDF letters*}
{if $form.template.html}
{if !empty($form.template.html)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -34,7 +34,7 @@
<td class="label">{$form.description.label}</td>
<td>{$form.description.html}</td>
</tr>
{if $form.group_type}
{if !empty($form.group_type)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -102,7 +102,7 @@

{* Existing Group *}

<div id="existing-groups" class="crm-accordion-wrapper crm-existing_group-accordion {if $form.groups} {else}collapsed{/if}">
<div id="existing-groups" class="crm-accordion-wrapper crm-existing_group-accordion {if !empty($form.groups)} {else}collapsed{/if}">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -228,7 +228,7 @@
</td>
</tr>
{/if}
{if $form.payment_processor_id}
{if !empty($form.payment_processor_id)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -135,7 +135,7 @@
{/if}
{/crmRegion}

{if $form.is_recur}
{if !empty($form.is_recur)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -199,7 +199,7 @@
<div class="crm-public-form-item crm-section honor_block_text-section">
{$honor_block_text}
</div>
{if $form.soft_credit_type_id.html}
{if !empty($form.soft_credit_type_id.html)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -256,7 +256,7 @@
{* end of ccid loop *}
{/if}

{if $form.payment_processor_id.label}
{if !empty($form.payment_processor_id.label)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -25,15 +25,15 @@
</td>
</tr>
<tr>
{if $form.contact_tags}
{if !empty($form.contact_tags)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -29,7 +29,7 @@
</div>

<div id="map" class="crm-section crm-export-mapping-section">
{if $form.mapping }
{if !empty($form.mapping)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -33,7 +33,7 @@
<tr class="crm-contact-task-addtogroup-form-block-description">
<td class="label">{$form.description.label}</td>
<td>{$form.description.html}</td></tr>
{if $form.group_type}
{if !empty($form.group_type)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -28,7 +28,7 @@
<td class="crm-participant-participant_role">{$details.role}</td>
</tr>
</table>
{if $form.contact_id}
{if !empty($form.contact_id)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -122,7 +122,7 @@
</fieldset>
{/if}

{if $form.payment_processor_id.label}
{if !empty($form.payment_processor_id.label)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -80,13 +80,13 @@
<td>{$form.dedupe_rule_group_id.html} {help id="id-dedupe_rule_group_id"}</td>
</tr>
<tr class="crm-event-manage-registration-form-block-requires_approval">
{if $form.requires_approval}
{if !empty($form.requires_approval)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -15,13 +15,13 @@
{include file="CRM/common/formButtons.tpl" location="top"}
</div>
<table class="form-layout-compressed">
{if $form.template_id}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -227,7 +227,7 @@

function fillTotalAmount( totalAmount ) {
if ( !totalAmount ) {
var amountVal = {/literal}{if $form.amount.value}{$form.amount.value}{else}0{/if}{literal};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -15,7 +15,7 @@
</td>
</tr>

<tr id="option_group" {if !$form.option_group_id}class="hiddenElement"{/if}>
<tr id="option_group" {if empty($form.option_group_id)}class="hiddenElement"{/if}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -30,7 +30,7 @@
<td class="label">{$form.serialize.label}</td>
<td class="html-adjust">{$form.serialize.html}</td>
</tr>
{if $form.in_selector}
{if !empty($form.in_selector)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -136,7 +136,7 @@
<div class="crm-amount-raised-wrapper">
<span id="crm_cpid_{$cpageId}_amt_raised" class="crm-amount-raised"> -- placeholder -- </span>
</div>
{if $form.url_logo.value}
{if !empty($form.url_logo.value)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -420,7 +420,7 @@

</script>
{/literal}
{if $form.is_recur}
{if !empty($form.is_recur)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@@ -13,7 +13,7 @@
* FIXME: This is way more complex than it needs to be
* FIXME: Why are we not just using the dynamic form tpl to display this profile?
*}
{if $form.is_for_organization}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked this file = ok

@eileenmcnaughton
Copy link
Contributor

OK - I think I've eyeballed them all. I definitely think fixing enotices is a chance to fix cruft - but OTOH there are a lot to work through so gonna merge this

@eileenmcnaughton eileenmcnaughton merged commit aa99ada into civicrm:master Oct 6, 2021
@eileenmcnaughton eileenmcnaughton deleted the tplFix branch October 6, 2021 05:46
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