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

Remove isset from tpl file #21939

Closed
wants to merge 2 commits into from

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fixes a file to not use isset per #21935. In one case empty does not have the same meaning & I used |smarty:nodefaults to tell smarty not to apply default modifiers - which escape on output would be

Before

Use of isset which is not compatible with escape on output

After

empty & |smarty:nodefaults

Technical Details

Per #21935 it turns out isset will be
a blocker to ever doing escape on output so switching a couple of places to
empty. In the last case empty doesn't cut it so I added smarty:nodefaults
so that it would not be escaped.

isset doesn't work on the out put from a function

Comments

@demeritcowboy this is the mere measured approach....

Per civicrm#21935 it turns out isset will be
a blocker to ever doing escape on output so switching a couple of places to
empty. In the last case empty doesn't cut it so I added smarty:nodefaults
so that it would not be escaped.

isset doesn't work on the out put from a function
@civibot
Copy link

civibot bot commented Oct 29, 2021

(Standard links)

@demeritcowboy
Copy link
Contributor

Ok. So my reading is:

The "escape on output" would be setting $this->default_modifiers below (which is part of smarty) via the line in the other PR https://github.com/civicrm/civicrm-core/pull/21935/files#diff-f364d50e57bb9a2a1a67ffb43bd20f297dd8115064f57b7c954c8c8c5117618fR144 so that it includes escape.

As per below using smarty:nodefault would then skip applying that, to allow for using isset on $tabValue.count which is needed because it might be 0, which empty() would get wrong.

So I think I get it now.

if(preg_match('~^(' . $this->_obj_call_regexp . '|' . $this->_dvar_regexp . ')(' . $this->_mod_regexp . '*)$~', $val, $match)) {
            // $ variable or object
            $return = $this->_parse_var($match[1]);
            $modifiers = $match[2];
            if (!empty($this->default_modifiers) && !preg_match('~(^|\|)smarty:nodefaults($|\|)~',$modifiers)) {
                $_default_mod_string = implode('|',(array)$this->default_modifiers);
                $modifiers = empty($modifiers) ? $_default_mod_string : $_default_mod_string . '|' . $modifiers;
            }
            $this->_parse_modifiers($return, $modifiers);
            return $return;

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy well done - you deserve a mind-altering beverage for getting your head around that :-)

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy EXCEPT - I turned on enotices & the empty ones turn out to need nodefaults too....

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I pushed in |smarty:nodefaults on the empty checks in the file too - obviously they are less important since enotices are less bad that crashes. However, since I had touched one in this commit I figured I'd do the file

@demeritcowboy
Copy link
Contributor

While this file is maybe a good example to see what's going on, it maybe isn't a good example to see the end goal of default being escaped, since now most of this file is littered with "nodefault" which is the same as the current default. It's mergeable in the sense it runs fine just now seems awkward code.
And I wonder if this works with php 8 since that was what adding all those empty()'s was about?

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy right - we added all of those emptys for php8 - without realising they wouldn't play nice if we wanted escaping on :-( It's phpunit8 that won't tolerate them I think.

My feeling is the right fix is probably to ensure that things are assigned & not rely on 'empty' - having said that I'm tearling my hair our right now trying to assign 'breadcrumb' from 'somewhere' so that it would never be unassigned - although it could be empty - in CMSPrint.tpl

In terms of escape on output it really is the 'isset' ones that cause it to 'not work' - the empty ones only mess with cases where we have enotice enabled for smarty

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy this is an example #21934 of where I think 'ensure-assigned' works well

@demeritcowboy demeritcowboy added merge ready PR will be merged after a few days if there are no objections and removed merge on pass labels Oct 29, 2021
@eileenmcnaughton
Copy link
Contributor Author

I'm gonna close this for now - I want to have another go & see if I can resolve it through better assigns rather than smarty:nodefaults & I don't really want it merged before I get to it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants