dev/core#4144 Fix smarty notice on isDuplicate #25657

eileenmcnaughton · 2023-02-23T23:59:56Z

Overview

dev/core#4144 Fix smarty notice on isDuplicate

Before

After

Technical Details

It was really hard to figure out the point of this variable / how it was reached. Most of the change is code commentary to try to make that clearer

The way isDuplicate comes into play is that duplicate checking is done if (& only if) the profile is used in the context of creating a new (e.g) individual from an entity reference field

Comments

civibot · 2023-02-23T23:59:58Z

No issue was found matching the number given in the pull request title. Please check the issue number.

civibot · 2023-02-23T23:59:59Z

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

eileenmcnaughton · 2023-02-24T00:02:57Z

CRM/Profile/Form.php

-   * Name of button for saving matching contacts.
-   * @var string
-   */
-  protected $_duplicateButtonName;


this just muddies the waters using a property

I agree, strange choice

eileenmcnaughton · 2023-03-08T00:57:05Z

This passed but I rebased to keep it fresh - seems like an infra issue test this please

highfalutin

The template could use one more little change to simplify it further

highfalutin · 2023-03-10T18:08:02Z

CRM/Profile/Form.php

-   * Name of button for saving matching contacts.
-   * @var string
-   */
-  protected $_duplicateButtonName;


I agree, strange choice

highfalutin · 2023-03-10T18:17:21Z

CRM/Profile/Form.php

@@ -261,6 +257,18 @@ protected static function handleDuplicateChecking(&$errors, $fields, $form) {
    return $errors;
  }

+  /**
+   * Is this being called from an entity reference field.


helpful explanation

highfalutin · 2023-03-10T18:34:28Z

templates/CRM/Profile/Form/Dynamic.tpl

@@ -35,7 +35,7 @@
  <div id="crm-container" class="crm-container crm-public" lang="{$config->lcMessages|truncate:2:"":true}" xml:lang="{$config->lcMessages|truncate:2:"":true}">
  {/if}

-  {if $isDuplicate and ( ($action eq 1 and $mode eq 4 ) or ($action eq 2) or ($action eq 8192) ) }
+  {if array_key_exists('_qf_Edit_upload_duplicate', $form) && $isDuplicate}
    <div class="crm-submit-buttons">
      {$form._qf_Edit_upload_duplicate.html}


Seems odd that we hard-code the button name here, and go to the trouble to generate it dynamically in Form.php... but if it works, fine

highfalutin · 2023-03-10T18:53:48Z

templates/CRM/Profile/Form/Dynamic.tpl

@@ -35,7 +35,7 @@
  <div id="crm-container" class="crm-container crm-public" lang="{$config->lcMessages|truncate:2:"":true}" xml:lang="{$config->lcMessages|truncate:2:"":true}">
  {/if}

-  {if $isDuplicate and ( ($action eq 1 and $mode eq 4 ) or ($action eq 2) or ($action eq 8192) ) }
+  {if array_key_exists('_qf_Edit_upload_duplicate', $form) && $isDuplicate}


Does this not change the logic?

($action eq 1 and $mode eq 4 ): ADD and MODE_CREATE
$action eq 2: UPDATE
$action eq 8192: PROFILE

I can't grok why we need to test for these; $isDuplicate should be enough, I'd think.

And array_key_exists('_qf_Edit_upload_duplicate', $form) should always be TRUE.

So can we just change this to {if $isDuplicate}?

@highfalutin the reason for the button check is that the button is ONLY there under a very narrow situation - if you use an entity reference field & choose (e.g) new Organization AND choose an organization that exists THEN the button appears

highfalutin · 2023-03-10T19:26:36Z

Now trying to do a run, but I can't find where to trigger these notices. What form should I go to?

eileenmcnaughton · 2023-03-10T21:54:17Z

@highfalutin they occur when using profiles - I'm not sure how intolerant your set up has to be but I saw another php 8.x user try to fix it & I definitely see it when I turn on smarty hardening - https://docs.civicrm.org/dev/en/latest/security/outputs/#html

highfalutin · 2023-03-11T00:45:28Z

OK, for me the notices only show up if I view the form in its own tab, instead of as a modal.

Smarty hardening doesn't need to be turned on for the notices to appear.

Side note: I was a bit confused by the mention of "entity reference" -- the special "New Individual" and "New Organization" links don't appear on Entity Reference/Contact Reference custom fields, only on built-in fields built with CRM_Core_Form::addEntityRef()... but anyway that turns out to be a red herring. The smarty notices actually seem to appear on any profile used in standalone Create/Edit mode, it doesn't really matter how you get to them.

Before

Open a profile edit page like civicrm/profile/create&reset=1&gid=xxxxx where xxxxx is the id of any profile which can be used in standalone create/edit mode.

Several notices appear, including:

Warning: Undefined array key "isDuplicate" in /path/to/civi.files/templates_c/en_US/%%0A/0AF/0AF6F87E%%Dynamic.tpl.php on line 35
Warning: Undefined array key "isDuplicate" in /path/to/civi.files/templates_c/en_US/%%0A/0AF/0AF6F87E%%Dynamic.tpl.php on line 86

The same notices appear when &context=dialog is added to the url, telling Civi that the output is intended for a modal dialog box.

After

In dialog context, both notices disappear. In non-dialog context, the notice about line 35 disappears but the one about line 86 remains.

To get both lines to disappear in both contexts:

diff --git a/CRM/Profile/Form.php b/CRM/Profile/Form.php
index 5bb2cf99d8..e8c8d9bdc4 100644
--- a/CRM/Profile/Form.php
+++ b/CRM/Profile/Form.php
@@ -904,10 +904,11 @@ class CRM_Profile_Form extends CRM_Core_Form {
       $this->freeze();
     }

+    // Assign FALSE, here - this is overwritten during form validation
+    // if duplicates are found during submit.
+    CRM_Core_Smarty::singleton()->assign('isDuplicate', FALSE);
+
     if ($this->isEntityReferenceContactCreateMode()) {
-      // Assign FALSE, here - this is overwritten during form validation
-      // if duplicates are found during submit.
-      CRM_Core_Smarty::singleton()->assign('isDuplicate', FALSE);
       $this->addElement(
         'xbutton',
         $this->getButtonName('upload', 'duplicate'),

I also took out the array_key_exists('_qf_Edit_upload_duplicate', $form) && from the template, and confirmed that everything works fine without it. $isDuplicate should be renamed to $showSaveDuplicateButton because that's really what it's about.

eileenmcnaughton · 2023-03-11T00:52:16Z

Thanks @highfalutin - I'll make the changes you suggest once I have fulfilled my chauffeur duties...

…nother couple notices

highfalutin · 2023-03-11T01:01:27Z

just made a PR to your PR 😆

fix notice in non-dialog mode too, make var name clearer, fix another notice

eileenmcnaughton · 2023-03-11T02:11:53Z

OH wow - I got sooo confused - I thought your PR was against master & then I thought this must be already merged but then it wasn't and then my son sang 'I'm Slim Shady' & my brain broke

eileenmcnaughton · 2023-03-11T02:12:53Z

I've given the merge-on-pass based on the review / collaboration from @highfalutin

highfalutin · 2023-03-11T02:39:37Z

@Eileen we'll have to credit Eminem in the next release 🤣

eileenmcnaughton · 2023-03-11T03:30:58Z

lol

civibot bot added the master label Feb 23, 2023

eileenmcnaughton commented Feb 24, 2023

View reviewed changes

dev/core#4144 Fix smarty notice on isDuplicate

5ae6078

eileenmcnaughton force-pushed the dynamic branch from 9e9ce4a to 5ae6078 Compare March 8, 2023 00:20

eileenmcnaughton mentioned this pull request Mar 9, 2023

Smarty notice fix on Merge #25772

Merged

highfalutin suggested changes Mar 10, 2023

View reviewed changes

eliminate notice in non-dialog mode too, make var name clearer, fix a…

81648fb

…nother couple notices

highfalutin mentioned this pull request Mar 11, 2023

fix notice in non-dialog mode too, make var name clearer, fix another notice eileenmcnaughton/civicrm-core#16

Merged

Merge pull request #16 from lemniscus/smarty-notices-profile-dynamic

b0d2a00

fix notice in non-dialog mode too, make var name clearer, fix another notice

eileenmcnaughton added the merge on pass label Mar 11, 2023

eileenmcnaughton merged commit a846d2e into civicrm:master Mar 11, 2023

eileenmcnaughton deleted the dynamic branch March 11, 2023 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#4144 Fix smarty notice on isDuplicate #25657

dev/core#4144 Fix smarty notice on isDuplicate #25657

eileenmcnaughton commented Feb 23, 2023 •

edited

Loading

civibot bot commented Feb 23, 2023

civibot bot commented Feb 23, 2023

eileenmcnaughton Feb 24, 2023

highfalutin Mar 10, 2023

eileenmcnaughton commented Mar 8, 2023

highfalutin left a comment

highfalutin Mar 10, 2023

highfalutin Mar 10, 2023

highfalutin Mar 10, 2023

highfalutin Mar 10, 2023

eileenmcnaughton Mar 10, 2023

highfalutin commented Mar 10, 2023

eileenmcnaughton commented Mar 10, 2023

highfalutin commented Mar 11, 2023

eileenmcnaughton commented Mar 11, 2023

highfalutin commented Mar 11, 2023

eileenmcnaughton commented Mar 11, 2023

eileenmcnaughton commented Mar 11, 2023

highfalutin commented Mar 11, 2023

eileenmcnaughton commented Mar 11, 2023

dev/core#4144 Fix smarty notice on isDuplicate #25657

dev/core#4144 Fix smarty notice on isDuplicate #25657

Conversation

eileenmcnaughton commented Feb 23, 2023 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Feb 23, 2023

civibot bot commented Feb 23, 2023

eileenmcnaughton Feb 24, 2023

Choose a reason for hiding this comment

highfalutin Mar 10, 2023

Choose a reason for hiding this comment

eileenmcnaughton commented Mar 8, 2023

highfalutin left a comment

Choose a reason for hiding this comment

highfalutin Mar 10, 2023

Choose a reason for hiding this comment

highfalutin Mar 10, 2023

Choose a reason for hiding this comment

highfalutin Mar 10, 2023

Choose a reason for hiding this comment

highfalutin Mar 10, 2023

Choose a reason for hiding this comment

eileenmcnaughton Mar 10, 2023

Choose a reason for hiding this comment

highfalutin commented Mar 10, 2023

eileenmcnaughton commented Mar 10, 2023

highfalutin commented Mar 11, 2023

Before

After

eileenmcnaughton commented Mar 11, 2023

highfalutin commented Mar 11, 2023

eileenmcnaughton commented Mar 11, 2023

eileenmcnaughton commented Mar 11, 2023

highfalutin commented Mar 11, 2023

eileenmcnaughton commented Mar 11, 2023

eileenmcnaughton commented Feb 23, 2023 •

edited

Loading