smarty.const wrongly evaluated for usage with comparison operators

[shopx]

This refers to the current fixes for PHP 8 see #605
simple dummy example
somewhere in php define('_THEME', 'light');
template

{if $smarty.const._THEME == 'dark'} dark mode selected {elseif $smarty.const._THEME == 'light'} light mode selected {/if}

expected output
light mode selected

but using current master with the PHP 8 fixes output is
dark mode selected

this happens due to generated php
see file attached
example.txt

ok, looks like the in #605 proposed change for constant handling was already built in
imo the way its done is wrong
and, and its seems to me that smarty should not be responsible for handling undefined constants
I think this should be done at the application layer in PHP or whatever
if an constant is not defined and the application developer knows that this can happen (should not) the developer is responsible to handle this situation
smarty should not hide this

[shopx]

see libs/sysplugins/smarty_internal_compile_private_special_variable.php
commit 818aa3c
should be
return "( defined('{$_index[1]}') ? constant('{$_index[1]}') : null )";
??

[shopx]

relates also logical operators #609

[harm-smits]

see libs/sysplugins/smarty_internal_compile_private_special_variable.php
commit 818aa3c
should be
return "( defined('{$_index[1]}') ? constant('{$_index[1]}') : null )";
??

Correct

[ophian]

and, and its seems to me that smarty should not be responsible for handling undefined constants
I think this should be done at the application layer in PHP or whatever

I think this is the main thing to focus on! And I had wondered why nobody came up with it yet. The #605 issue is just pointing wrong since PHP 7.4+ was getting more strict regarding constants. Smarty is not a layer to fix up things like this 818aa3c. Wrong usage of constant is just wrong usage and should not get "fixed" by an interpreter.

IMHO, the right Smarty usage for null Constants, besides Constants not being meant to be used like dynamic assigned VARIABLES and in conditional structures like if / else, and when you are not sure it is defined at all, is this:

{$smarty.const.TEST|default:''} or {$smarty.const.TEST|default:$smarty.const.ANOTHER}

So regarding all mentioned examples I would recommend to assign real variables instead and remove this misleading fix 818aa3c. that indeed would need the parenthesis fixup like already mentioned.

[harm-smits]

@ophian Backwards compatability is also a thing you know.

[ophian]

@harm-smits I don't see what you mean by this. Backward compat of wrong usage?
If you really need to check constants as conditionals do {if ($smarty.const.TEST|default:'') == 'dark'} do something {/if} Or write an 'isDefined' modifier or function. Doing this in conditionals in PHP with plain constants is a wrong usage too. PHP is just getting more strict, which is a good thing and we don't need backward compat at this point here.
I would always plead for that Devs fixup old code in their applications, before making Smarty fall back.

[harm-smits]

Seeing as Smarty is still compatible with PHP 5.2, I'd say backwards compatability (even of wrong usage), is essential. You don't just change behaviour for no apparant reason. This instead should be done for smarty version 4. Changing error levels is something that is considered a major change after all.

[ophian]

I did not talk about changing error levels. This is a second thing in the commit and is done for unit tests only.
I can't follow with 5.2 though. Someone is not going to implement Smarty 3.1.x or 3.2 for elder PHP 5 Series systems. So nothing changes, does it?

Changing error levels is something that is considered a major change after all.

In this case you could not use PHP 7/8, since they do. 😉

[harm-smits]

What are you on? You just stated, you want undefined consts, to now cause a 'Notice' of some sorts, which is per definition changing error levels and previous behaviour. Also insinuating that different behaviour for different PHP versions should be a thing is just not okay. This causes inconsistency and will only cause confusion for those using smarty and expecting it to at least behave the same on different PHP versions.

[ophian]

Well Yes, and No, not really. I stated, that I want Smarty to behave like PHP. If the latter changes error/notice reporting for general improvement, for when someone uses code falsely, I would like to have this happen equally. The commit tries to fix this in my eyes.
When Smarty is trying to suppress these messages because PHP improved and now does alert, things get never fixed and never change. PHP 7/8 had major changes. If you update this parent this should get transported and not fall back to some 5.2 behaviour.

So it is more a matter of, if Smarty should be trying to be more smarter than the parent system it depends on, or not, ...even if "Smarty" and "Smart" do have the same wording root 😄 ...and we all like its easiness.

This is nothing to get into conflicts. Its a position, and I can understand yours too. I just think having a Smarty defined ternary on constant defines will never show the cause and that something IS wrong, or even wrongly used, and which (as stated) could even in Smarty get an easy fix - if you get it into attention. I fixed a lot of old systems and templates because of things like this, so I know where I come from. So for my view it is more a call to the Devs to get their work done! Be precise and not lazy as PHP was in its younger days. Show your Smarty using Users how to do it right! Thats all.

[liborm85]

Prepared PR with fix #618.