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

Smarty - {htxt} blocks should not be evaluated unless needed #25653

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

totten
Copy link
Member

@totten totten commented Feb 23, 2023

Overview

Fixes a common class of PHP warning about *.hlp files.

Screen Shot 2023-02-23 at 2 01 56 PM

Before

The {htxt} block appears to act like a conditional expression. If you look at function.htxt.php, it checks the value of $id and conditionally enables or disables the content.

But it is not an effective conditional. The nested content is evaluated before it calls smarty_block_htxt().

Consequently, if you try to show help for field foo, then it actually evaluates the help message for everything in the .hlp file (even if it's some quirky field that's not relevant and that requires extra data).

After

The {htxt} is literally like an {if}.

You don't get warnings about irrelevant data.

@civibot
Copy link

civibot bot commented Feb 23, 2023

(Standard links)

@colemanw
Copy link
Member

Looks good.

@eileenmcnaughton
Copy link
Contributor

@totten not so fast

CRM_Case_Form_EmailTest::testOpeningEmailForm
Smarty error: [in CRM/Contact/Form/Task/Help/Email/id-from_email.hlp line 3]: syntax error: mismatched tag {/if}. expected {/crmScope} (opened line 1). (Smarty_Compiler.class.php, line 2337)

/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1100
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1860
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty_Compiler.class.php:2255
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty_Compiler.class.php:2337
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty_Compiler.class.php:486
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty_Compiler.class.php:306
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1499
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1432
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1271
/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/CRM/Core/Smarty.php:193

@@ -144,6 +144,7 @@ private function initialize() {
$this->default_modifiers[] = 'escape:"htmlall"';
}
$this->load_filter('pre', 'resetExtScope');
$this->load_filter('pre', 'htxtFilter');
Copy link
Contributor

Choose a reason for hiding this comment

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

as an aside - from memory this is one thing I am still stuck on with smarty 3 - I think this way of loading filters changes & I haven't figured out what to do instead

Copy link
Member Author

@totten totten Feb 24, 2023

Choose a reason for hiding this comment

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

To my eye, the contract for pre-filters looks pretty similar in v2 and v3. I suppose the name loadFilter() is preferred in v3, but it looks like load_filter() is still provided (via SmartyBC).

@totten totten force-pushed the master-htxt branch 3 times, most recently from fcef51b to 6b68a5f Compare February 24, 2023 01:23
@totten
Copy link
Member Author

totten commented Feb 24, 2023

CRM_Case_Form_EmailTest::testOpeningEmailForm
Smarty error: [in CRM/Contact/Form/Task/Help/Email/id-from_email.hlp line 3]: syntax error: mismatched tag {/if}. expected {/crmScope} (opened line 1). (Smarty_Compiler.class.php, line 2337)

Right-o. It was going line-by-line and failed with certain extra bits at the start of line (eg {foo}{htxt id=...). Switched to preg_replace_callback(), and that should work better. (This test passes, anyway.)

Also corrected a typo in the new error message.

@eileenmcnaughton eileenmcnaughton merged commit 273f4f4 into civicrm:master Feb 24, 2023
@totten totten deleted the master-htxt branch February 24, 2023 05:20
@agileware-justin
Copy link
Contributor

Reported regression, https://lab.civicrm.org/dev/core/-/issues/4303

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