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 common templates to assit with fixi… #20543

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

seamuslee001
Copy link
Contributor

…ng test failures on php8

Overview

This adds in some if guards into various common templates to assist with some test failures in php8

Before

Tests can fail because of issues in templates

After

Tests fail for different reasons

ping @eileenmcnaughton @totten @demeritcowboy @colemanw

@civibot
Copy link

civibot bot commented Jun 8, 2021

(Standard links)

@colemanw
Copy link
Member

Code looks good.

@seamuslee001 seamuslee001 deleted the guards_common branch June 11, 2021 02:38
@@ -9,7 +9,7 @@
*}
{* Initialize jQuery validate on a form *}
{* Extra params and functions may be added to the CRM.validate object before this template is loaded *}
{if !$crm_form_validate_included and $smarty.get.snippet neq 'json' and $form and $form.formClass}
{if empty($crm_form_validate_included) and !empty($smarty.get.snippet) and $smarty.get.snippet neq 'json' and !empty($form) and !empty($form.formClass)}
Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 I think this line isn't intended to change the behavior of the if statement, but it does - this prevents crmValidate() from being loaded on event registration pages:

and !empty($smarty.get.snippet)

This change causes the conditional to fail on event registration pages where previously it succeeded. I haven't found a scenario where that directly affects core, but it breaks CiviHoneypot and I wouldn't be surprised if there were configurations that could break core as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MegaphoneJon you are right - this looks like an unintentional change

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.

4 participants