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

[php8-compact] Add in guards into various templates to fix the CRM_Co… #20579

Merged

Conversation

seamuslee001
Copy link
Contributor

…ntact_Form_IndividualTest suite

Overview

This adds in a number of if guards to fix the CRM_Contact_Form_IndividualTest suite on php8

Before

CRM_Contact_Form_IndividualTest fails on php8

After

CRM_Contact_Form_IndividualTest passes on php8

ping @demeritcowboy @totten @colemanw

@civibot
Copy link

civibot bot commented Jun 11, 2021

(Standard links)

@civibot civibot bot added the master label Jun 11, 2021
@seamuslee001 seamuslee001 force-pushed the fix_CRM_Contact_Form_IndividualTest branch from 3a1c836 to 560b4f3 Compare June 11, 2021 12:13
<tr>
{if !$type || $type eq 'tag'}
{if empty($type) || (isset($type) && $type eq 'tag')}
Copy link
Member

Choose a reason for hiding this comment

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

The second guard is redundant because after the || we know type is not empty and therefore isset.

Suggested change
{if empty($type) || (isset($type) && $type eq 'tag')}
{if empty($type) || $type eq 'tag'}

{$form.tag.html}
</div>
{if $context NEQ 'profile'}
{if !isset($context) || (isset($context) && $context NEQ 'profile')}
Copy link
Member

Choose a reason for hiding this comment

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

The second guard is redundant because after the || we know $context is set.

Suggested change
{if !isset($context) || (isset($context) && $context NEQ 'profile')}
{if !isset($context) || $context NEQ 'profile'}

@@ -22,7 +22,7 @@
<div id="Address_Block_{$blockId}" {if $className eq 'CRM_Contact_Form_Contact'} class="boxBlock crm-edit-address-block crm-address_{$blockId}"{/if}>
{if $blockId gt 1}<fieldset><legend>{ts}Supplemental Address{/ts}</legend>{/if}
<table class="form-layout-compressed crm-edit-address-form">
{if $masterAddress.$blockId gt 0 }
{if isset($masterAddress) && $masterAddress.$blockId gt 0 }
Copy link
Member

Choose a reason for hiding this comment

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

I'll be you anything the IDs don't go into negative numbers and the gt was just another way of saying

Suggested change
{if isset($masterAddress) && $masterAddress.$blockId gt 0 }
{if !empty($masterAddress.$blockId) }

But your version is a more literal interpretation of what was already there so probably safer.

{include file="CRM/common/Tagset.tpl"}
{/if}
</td>
{/if}
{if !$type || $type eq 'group'}
{if isset($type) && $type eq 'group'}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's exactly the same. I think it should be

Suggested change
{if isset($type) && $type eq 'group'}
{if empty($type) || $type eq 'group'}

Disclaimer: this is making my brain hurt so I could be wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

Markdown table to the rescue!

The integer 0 one is fun.

type {if !$type || $type eq 'group'} {if isset($type) && $type eq 'group'} {if empty($type) || $type eq 'group'}
unset TRUE FALSE TRUE
null TRUE FALSE TRUE
group TRUE TRUE TRUE
banana FALSE FALSE FALSE
"" TRUE FALSE TRUE
"0" TRUE FALSE TRUE
"1" FALSE FALSE FALSE
0 TRUE TRUE TRUE
1 FALSE FALSE FALSE
TRUE TRUE TRUE TRUE
FALSE TRUE FALSE TRUE

Copy link
Member

Choose a reason for hiding this comment

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

LOL, you never know when you expect a group but it's actually a banana.

@seamuslee001 seamuslee001 force-pushed the fix_CRM_Contact_Form_IndividualTest branch from 560b4f3 to e2e6000 Compare June 11, 2021 21:19
@seamuslee001
Copy link
Contributor Author

Thanks @colemanw @demeritcowboy I believe I have fixed those issues mentioned in the TagsAndGroups tpl file which seemed to be the only one of concern.

@seamuslee001 seamuslee001 merged commit 45228cf into civicrm:master Jun 11, 2021
@seamuslee001 seamuslee001 deleted the fix_CRM_Contact_Form_IndividualTest branch June 11, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants