(dev/core#4303) Allow {htxt} guard to accommodate variable ID #26298
Conversation
Before
------
`{htxt}` guard works with static IDs (`{htxt id="abc"}`)
After
-----
`{htxt}` guard with with variable ID (`{htxt id=$abc}`)
Comments
--------
As discussed in dev/core#4303, one relevant use-case comes from `CustomField.hlp`.
In my copy of universe, there's a similar construction in
`nz.co.fuzion.entitysetting` (`entitysettingscommon.hlp`).
|
(Standard links)
|
|
Note: You will probably need to flush caches if applying the patch directly (without a version-change or upgrade). |
| @@ -22,6 +22,10 @@ function smarty_prefilter_htxtFilter($tpl_source, &$smarty) { | |||
| $htxts++; | |||
| return sprintf('{if $id == %s}%s', $m[1], $m[0]); | |||
| }, | |||
| '/\{htxt id=(\$\w+)}/' => function ($m) use (&$htxts) { | |||
There was a problem hiding this comment.
I think this would not match {htxt id=$foo otherstuff}, which seems like it should be valid. Seems safest just to remove that }.
There was a problem hiding this comment.
@larssandergreen That's correct, it doesn't/didn't match otherstuff.
Regarding this specific example, we could treat either } or as terminating the id=... phrase. This would allow subsequent parameters.
I wouldn't exactly drop the }. Consider text like {htxt id=$foo.bar} or {htxt id=$foo[bar]} or {htxt id=$foo"bar"} or {htxt id=$foo{bar}}:
- If the regex has no closing marker (
id=(\$\w+)), then it does a partial match -- matchingid=$fooand ignoring.baror[bar]or"bar"or{bar}. I expect that to produce some weird bug. - If the regex has a closing marker (
id=(\$\w+)[} ]), then it will not match any of those examples.htxtFilterwill throw an exception (Invalid {htxt} tag), which points you to the right element.
Within the world of "unsupported edge-cases", the explicit error is better, no?
Anyway, the example you gave ( otherstuff) should be fairly easy to accommodate by accepting as another closing-marker. I'm pushing up a commit to do that.
|
Looks good to me other than that one detail, but the details of the test are over my head so I can't comment on that. |
Also: tighten the handling of ' and " to ensure that they are matched properly.
|
OK - that looks like a thumbs up from @larssandergreen after some good review - merging @totten we should push out... |
|
@totten If you're pushing a new point version for this, I think it would make sense to include this related fix #26079 (which is actually worse in terms of impact, I think, see this recent issue). |
|
Good point @larssandergreen - if that related PR had been included in the last CV point release then the problem would have not been triggered in our case or that of the related gitlab. |
|
Ooph, yeah, agree that flavor of the bug is more severe. It's included in 5.61 PR. |
Ovreview
Fix recent regression (reported in https://lab.civicrm.org/dev/core/-/issues/4303) where
CustomField.hlpgenerates an error while processing{htxt}.Before
{htxt}guard works with static IDs ({htxt id="abc"})After
{htxt}guard with with variable ID ({htxt id=$abc})Comments
In my copy of universe, there's a similar construction in
nz.co.fuzion.entitysetting(entitysettingscommon.hlp). However, in that case, it appears to loop through a list of settings and (conditionally) generates help blobs for each (but ultimately only needs to display one). I haven't actuallyr-runthat extension, but I'd be hopeful that this addresses it as well.