Smarty - When temporarily setting variable, fix cleanup for previously-undefined values.

[totten]

Overview

You can temporarily assign Smarty values using various methods (pushScope($vars)/popScope() or fetchWith(...$vars) or {crmScope ...}). This expands the test-coverage for that mechanism.

Before

Fewer tests

After

Mo tests, mo problems.

Comments

Test expected to fail. Off-shoot from discussion with @demeritcowboy on #26432.

(Updated)

I've added a patch to fix the demonstrated failure.

[civibot]

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

[monishdeb]

@totten seems like test failure is related:

CRM_Core_SmartyTest::testFetchWith_CleanNonExistent
Failed asserting that true is false.

/home/homer/buildkit/build/build-3/web/sites/all/modules/civicrm/tests/phpunit/CRM/Core/SmartyTest.php:36
/home/homer/buildkit/build/build-3/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:238
/home/homer/buildkit/extern/phpunit8/phpunit8.phar:1721

[totten]

@monishdeb Yup, that was supposed to demonstrate the failure. I just pushed up the actual fix. Hopefully, fixing that doesn't cause massive trouble elsewhere...

[demeritcowboy]

CRM_Core_Smarty_plugins_CrmScopeTest.testBlank with data set #2
CRM_Core_Smarty_plugins_CrmScopeTest.testBlank with data set #4
CRM_Core_Smarty_plugins_CrmScopeTest.testBlank with data set #9

"Undefined array key 'x'"

I'm not clear why those haven't always failed. Possibly adding the useTransaction() stops the errors from getting swallowed somewhere. The fails are just a variation of all the warnings about smarty variables that never get assigned. So either, can't do those test cases, or the test would need updating to expect a php warning but do we really care to enforce that.

Also case 9 is the same as case 2. Probably some history there, or maybe it was testing that you could repeat a test and still get the same result after all the other pushing and popping.

[totten]

I'm not clear why those haven't always failed.

Right, I'm pretty sure it was a leak between test-cases. Consider case #1 ({crmScope x=1}{/crmScope}):

• From global POV, you start with x undefined
• Then temporarily set x=1
• Then cleanup -- but the cleanup was flawed. It set x=null instead of unassigning x.

For all subsequent tests, they start with global x=null - so there's no notice about undefined x. The patch here changes the behavior -- subsequent tests now begin with x undefined. Hence, more warnings.

(One way to confirm the interaction -- checkout the older code, then play around with disabling or reordering specific $cases.)

(This suggests that CRM_Core_Smarty is currently a vector for other leaks... but that's a separate issue...)

expect a php warning but do we really care to enforce that.

Yeah, agree that doesn't need to be enforced.

We could define the global/default value. Since x uses a sequence (x=1, x=2, x=3) for each level of nesting, we could simply start with a global x=0. It doesn't provide any coverage for nullity, but 6996cbb does.

Also case 9 is the same as case 2. Probably some history there, or maybe it was testing that you could repeat a test and still get the same result after all the other pushing and popping.

Oh, I like that explanation... :)