Fixed PHP 8 modifier "default"

[juvenrui]

PHP 8 has deprecated for "@". But modifier "default" is not compatible.

plugins/modifiercompiler.default.php
// $output = '(($tmp = @' . $output . ')===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
$output = '(($tmp = ' . $output . ' ?? null)===null||$tmp===\'\' ? ' . $param . ' : $tmp)'; // edit by 2020.12.1 -- fixed PHP 8

[scottchiefbaker]

I don't see @ being deprecated. Where did you see that?

[juvenrui]

I don't see @ being deprecated. Where did you see that?

Really, "The @ operator will no longer silence fatal errors ......" (https://www.php.net/manual/zh/migration80.incompatible.php)

thanks!

[timmit-nl]

maybe it is time for smarty v4 so it can shake of any support for pre php7.2. As it whould be a challange to get full compatibility for php8 en below 7.1 (and those are all EOL anyway)

[Bigbear7]

Well, I think given that PHP 8 is out now, it would be good with a 3.1.37 release that fixes the most urgent hiccups. For a Smarty 4 version I think that PHP 7.4 as a minimum could be an idea given features like typed properties etc.

[omexlu]

Does smarty run on php8 already? :)

[juvenrui]

Does smarty run on php8 already? :)

Yes, version is 3.1.36，PHP8 is run OK! 😀

[chriswheeler]

3.1.36 doesn't work for me with PHP8, it needs some fixes which appear to be on master (c295786) - is there an ETA for 3.1.37?

[omexlu]

3.1.36 doesn't work for me with PHP8, it needs some fixes which appear to be on master (c295786) - is there an ETA for 3.1.37?

What exactly is not working for you? I have not noticed any problems under php 8.0.0 so far?

[chriswheeler]

I get this error from a smarty generated compiled template:

PHP Fatal error: Uncaught ArgumentCountError: Too few arguments to function Smarty_Internal_ErrorHandler::mutingErrorHandler(), 4 passed and exactly 5 expected in /home/redacted/public_html/system/composer/vendor/smarty/smarty/libs/sysplugins/smarty_internal_errorhandler.php:68

The line in the .tpl.php file which caused this was calling a static method of a class registered with smarty->registerClass()

[omexlu]

I dont have any issues with php8 until now.

[bhsmither]

chriswheeler, were you asking for a fix? Or an ETA on a published fix? You may have already seen this, but please see:
e128953
https://www.php.net/manual/en/function.set-error-handler.php errcontext warning

[chriswheeler]

@bhsmither just an ETA on when that fix will make it into a release - 3.1.37 I assume?

[timmit-nl]

Just to add to the topic starter:

$output = '(($tmp = ' . $output . ' ?? null)===null||$tmp===\'\' ? (' . $param . ' ?? null) : $tmp)'; // edit by 2020.12.1 -- fixed PHP 8 (https://github.com/smarty-php/smarty/issues/617)

has support for more default options and those also can be unset. So it is the same fix you did only also for the params:
if the array/object/value is empty or not set at all, this will work without any errors:

{$array.layer1.DT_KVERVAL|default: $array.DT_KVERVAL: ''}
(the multiple default params are undocumented, but existing in the original code)

we currently extend the template_plugin_modifiercompiler_default so we are not raising any errors on airbrake as it is also ignoring @.

It is NOT pre 7.0 supported as the ??(Null coalescing operator) is 7.0+ (https://www.php.net/manual/en/migration70.new-features.php#migration70.new-features.null-coalesce-op)

Maybe it is a good think to add some kind of legacy switch (maybe auto set in constructor) and diffentiate some code on that. So there is pre 7.0 support.

[wisskid]

This seems to be a duplicate of #605

[wisskid]

@timmit-nl sorry, I should have closed the other one as that one was already fixed. Reopening this one.

[timmit-nl]

no problem. I can make a merge request, but it is not compatible with php below 7.0. So there must be some kind of discussion before that how to handle legacy php. Does smarty drop it for 4.x.x (so if you want to use legacy php, use 3.x.x.) Or make we an legacy switch and check if we an a legacy version and serve a different version?

[wisskid]

I plan to drop support for php below 7.1 or 7.2 in Smarty 4 to be able to support PHP8. Simon

…

Op 7 jan. 2021 om 14:23 heeft Tim Schoondergang ***@***.***> het volgende geschreven:  no problem. I can make a merge request, but it is not compatible with php below 7.0. So there must be some kind of discussion before that how to handle legacy php. Does smarty drop it for 4.x.x (so if you want to use legacy php, use 3.x.x.) Or make we an legacy switch and check if we an a legacy version and serve a different version? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

[juvenrui]

Upgrade v3.1.37, the PHP8 output variable throw: 'Warning: Undefined array key "blabla"'

I have set a custom error handler function with set_error_handler():

ini_set('display_errors', '1');

set_error_handler(function ($severity, $message, $file, $line) {
    if ((error_reporting() & $severity) !== $severity) {
        return false;
    }
    throw new \ErrorException($message, 0, $severity, $file, $line);
});

I think, modifiercompiler.default.php use '@', set_error_handler() will still get called (https://www.php.net/manual/en/language.operators.errorcontrol.php).

My temporary solution, edit libs/plugins/modifiercompiler.default.php:

//$output = '(($tmp = @' . $output . ')===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
if (version_compare(PHP_VERSION, '7.0.0') >= 0) {
    $output = '(($tmp = ' . $output . ' ?? null)===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
} else {
    $output = '!isset(' . $output . ') || ' . $output . ' === null || ' . $output . ' === \'\' ? ' . $param . ' : ' . $output;
}

[timmit-nl]

Upgrade v3.1.37, the PHP8 output variable throw: 'Warning: Undefined array key "blabla"'

I have set a custom error handler function with set_error_handler():

ini_set('display_errors', '1');

set_error_handler(function ($severity, $message, $file, $line) {
    if ((error_reporting() & $severity) !== $severity) {
        return false;
    }
    throw new \ErrorException($message, 0, $severity, $file, $line);
});

I think, modifiercompiler.default.php use '@', set_error_handler() will still get called (https://www.php.net/manual/en/language.operators.errorcontrol.php).

My temporary solution, edit libs/plugins/modifiercompiler.default.php:

//$output = '(($tmp = @' . $output . ')===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
if (version_compare(PHP_VERSION, '7.0.0') >= 0) {
    $output = '(($tmp = ' . $output . ' ?? null)===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
} else {
    $output = '!isset(' . $output . ') || ' . $output . ' === null || ' . $output . ' === \'\' ? ' . $param . ' : ' . $output;
}

if you use for the 7.0 modifier
$output = '(($tmp = ' . $output . ' ?? null)===null||$tmp==='' ? (' . $param . ' ?? null) : $tmp)';

//$output = '(($tmp = @' . $output . ')===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
if (version_compare(PHP_VERSION, '7.0.0') >= 0) {
    $output = '(($tmp = ' . $output . ' ?? null)===null||$tmp===\'\' ? (' . $param . ' ?? null) : $tmp)'; 
} else {
    $output = '!isset(' . $output . ') || ' . $output . ' === null || ' . $output . ' === \'\' ? ' . $param . ' : ' . $output;
}

you can handle also unset default fallback

[wisskid]

@timmit-nl please see #629

[wisskid]

At everyone here: please give #629 a go and tell me what you think.

[chriswheeler]

With PHP 8.0.1 and smarty/dev-feature/php8-support:

|default now works well, without any PHP messages logged, so this now works:

{$request.get.location|default:$superglobal.session.location|default:'Default location'|htmlspecialchars}

However, I have quite a few places where I have instead something like this:

{if $request.get.location}{$request.get.location|htmlspecialchars}{else}{$superglobal.session.location|htmlspecialchars}{/if}

Which now give the following message:

PHP Warning: Undefined array key "location"

Prior to PHP8 there were no log messages. Was this intended to be fixed in #629?

If not I think it should be for Smarty 4 somehow, otherwise I have a lot of templates to update!

[wisskid]

@chriswheeler I'll have a look

[bhsmither]

Is the solution to this issue would be to fix "Undefined array key"?

I am coming across this with PHP8 and, for example, this template code:

<input type="text" name="global[dbhost]" id="form-dbhost" value="{$FORM.global.dbhost}" class="textbox required" />

where $FORM is not as yet assigned anything in the script.

Once compiled, the output to the browser aborts at:

<?php echo $_smarty_tpl->tpl_vars['FORM']->value['global']['dbhost'];?>

and the errors:

Warning 	Undefined array key "FORM"
Warning 	Attempt to read property "value" on null
Warning 	Trying to access array offset on value of type null
Warning 	Trying to access array offset on value of type null

[juvenrui]

{$FORM.global.dbhost}

try this: {$FORM.global.dbhost|default:''}

[bhsmither]

I could, but I am asserting that this - compiling/rendering a non-existsent variable object - wasn't any kind of problem under PHP7.4.

However, that particular installation of a program using Smarty running under PHP7.4 also had muteExpectedErrors() and a proper error_handler and exception_handler.

This problem shows up running Smarty's Demo under PHP8 - having added that <input> tag as a test.

[timmit-nl]

we have those errors also under 7.4 and lower. Therefore we use |default:'' everywhere.

Maybe because we have the higest error reporting setting, so get all errors. We like an errorless code, also without any notices or other not breaking errors ;-)

[Bigbear7]

Well, I just upgraded to Smarty 3.1.38 for our testsite that runs PHP 8.0. And in the logs we get:

• PHP Warning: Undefined array key

On PHP 7.4 that our production site runs we don't get this error.

[chriswheeler]

Yes, the issue here stems from PHP8 converting a number of previous Notices into Warnings - https://www.php.net/manual/en/migration80.incompatible.php

I suppose the questions is - is it the responsibility of Smarty to change the PHP code it generates to maintain the existing behaviour, or template authors to change their template code?

[Bigbear7]

For sure that is the case and the vote for that specific case was 42 - 21 in the RFC Reclassifying engine warnings, so one vote...

It's a fair question to ask whether it's a Smarty or the application using smarty responsiblity. For me in order to judge that I need to look into our own code a bit more. This code has been running flawlessly for quite some time, so hopefully there is a smooth way around it.

Anyway, typically the compiled smarty code that flunks has typically the structure, where the 'key' is undefined:

• $_smarty_tpl->tpl_vars['key']->value

[bhsmither]

I could concur that using |default:'' everywhere is a fine solution. But if this is used 'everywhere' whether it is needed or not, whether there is an expected null-ness or not expected, I would wonder why Smarty doesn't do this for us already.

[timmit-nl]

but what should be the default value be? null, false, ''? and where to stop?

ok if we all want to, I can life with a default null. I think we need to modify smarty that everywhere where $xxxxxx stands it is replace it with ($_smarty_tpl->tpl_vars['xxxxxx']->value ?? null).
I don't know the smarty classes good enough to say this can be done easy.

EDIT:

OK I have done some digging: I think we need to change the return of the function compileVariable from '$_smarty_tpl->tpl_vars[' . $variable . ']->value' to '($_smarty_tpl->tpl_vars[' . $variable . ']->value ?? null)' in smarty/libs/sysplugins/smarty_internal_templatecompilerbase.php

    /**
     * compile variable
     *
     * @param string $variable
     *
     * @return string
     */
    public function compileVariable($variable)
    {
        if (!strpos($variable, '(')) {
            // not a variable variable
            $var = trim($variable, '\'');
            $this->tag_nocache = $this->tag_nocache |
                                 $this->template->ext->getTemplateVars->_getVariable(
                                     $this->template,
                                     $var,
                                     null,
                                     true,
                                     false
                                 )->nocache;
            // todo $this->template->compiled->properties['variables'][$var] = $this->tag_nocache | $this->nocache;
        }
        return '($_smarty_tpl->tpl_vars[' . $variable . ']->value ?? null)';
    }

and go from there.... Only question: does the smarty team wants this. And the questions is maybe more: should smarty take care of the errors created by the programmer of the template (an undefined var in normal php code gives the same error...)

[bhsmither]

I would hope that muteExpectedErrors() would take care of the (now) Warnings for undefined variables and properties. And I, personally, would add an undefined key as being similar to an undefined variable. But none of the other "Notices are now Warnings" (http://docs.php.net/manual/en/migration80.incompatible.php).

I believe PHP does not stop running the script on Warnings -- which my assertion is that executing compiled/cached Smarty template scripts does stop running (i.e., an 'abend') under PHP8.

I also think that null, false, or a zero-length-string, in this context, are distinctions with no difference. None of them contribute anything to the output string.

[Bigbear7]

No, the application and compiled smarty scripts doesn't stop running, but the logs are flooded with warnings. Anyway, my quick & dirty fix for this is to mute warnings in smarty with:

• $smarty->setErrorReporting(E_ALL & ~E_WARNING & ~E_NOTICE);

[wisskid]

@bhsmither muteExpectedErrors has been deprecated since 2017, so I'm considering removing it, along with Smarty_Internal_ErrorHandler because Smarty no longer uses @filemtime() calls. We could re-use the concept though and activate it by default for PHP8 so Undefined array key and trying to read property of null warnings will be rethrown as notices.

[wisskid]

#629 now includes support for $smarty->setPHP7CompatMode() that will make Smarty ignore E_WARNINGS for undefined variables, array keys, et cetera. Rethrowing them as E_NOTICES is not possible. A side-effect is that some errors occurring in business logic called from the template source will also be silenced.
Therefore, I figured it would be better to not enable this by default. It's not very pretty, but it does the trick, I think.

[chriswheeler]

#626 now works well for me under PHP8 with no log lines generated - after I'd turned on setPHP7CompatMode().

It's a minor point, but I'm not sure that's the best naming, It might make people think they have to have it turned on for Smarty to be compatible with PHP7...

Would it be better to just tweak muteExpectedErrors() - rather than removing it and replacing it with setPHP7CompatMode()?

Both do something very similar, and anyone who was using muteExpectedErrors() (as I was) would probably also now want to use setPHP7CompatMode(). I can see my self simply replacing muteExpectedErrors() with setPHP7CompatMode() on a number of sites as I upgrade them to PHP8.

[bhsmither]

I vote for tweaking muteExpectedErrors() to also do what setPHP7CompatMode() would do - if practicable.

I regularly run into situations where access to the source PHP code is not allowed or simply not possible.

Which supports the idea of creating corresponding Smarty in-template commands - but that's a different issue.

[Bigbear7]

Regarding naming I wonder if this is more related with PHP 8 rather then PHP 7, so should it be called $smarty->setPHP8CompatMode() if not muteExpectedErrors()? But maybe it should handle other PHP 7 aspects?

Anyway, for our site smarty 3.1.36 runs flawlessly on it with PHP 7.4.

[tzcsx]

I am migrating a legacy application to PHP 8 and running into the mentioned Undefined array key E_WARNINGs. I am aware of the approach Smarty 4 has taken but unfortunately I can not upgrade to Smarty 4. I like @timmit-nl 's approach of patching Smarty's code generation using the null coalescing operator. However the suggested change to compileVariable does not quite work.
{$foo.bar} is compiled to ($_smarty_tpl->tpl_vars['foo']->value ?? null)['bar'] which will not work. Expected output is ($_smarty_tpl->tpl_vars['foo']->value['bar'] ?? null). I am currently digging into Smarty to adapt this myself but would really appreciate any hints.
Cheers

[timmit-nl]

the suggested change isn't tested and I have only written it as an example. We use everywhere |default: xx} as we want to know if something is not set....

[tzcsx]

Thanks for jumping back into this, timmit-nl. Yes of course the default filter would be a proper solution but introducing this into the legacy code base I have to work with unfortunately is not feasible. Your example code actually does work for {$foo} which is why I am motivated to adapt it to work with {$foo.bar} as well.

[tzcsx]

To my humble understanding of the grammar in lexer/smarty_internal_templateparser.y this approach is not going to work. Due to the object and array style nesting capabilities of Smarty, I would need to introduce a new expression to match a complete nesting statement. Guess this finally means EOL for legacy applications. Still very much open to suggestions.

[wisskid]

The setPHP7CompatMode method has been renamed due to popular demand in #629 :)

I'm not in favor of reusing the deprectated muteExpectedErrors() as that did something completely different.

Planning to release #629 as Smarty 4 soon (tm), unless someone encounters serious problems testing on php8 before.

[chriswheeler]

Note to future self when searching for this: setPHP7CompatMode() was renamed to muteUndefinedOrNullWarnings()

[Bigbear7]

Well, I have tested the php8-support branch and it runs nicely!

However, I had some HTML code within comments () and somehow it looks like it got parsed. And of course that generated a lot of warnings that was relevant. The warnings vanished once I removed the commented HTML code.

Another small comment is that on line 644, 1380 & 1390 in Smarty.class.php is says PHP7 compatibility. Think it should be PHP8 compatibility.