bcit-ci / CodeIgniter

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
18.27k stars 7.61k forks source link

@$variable is not ignored [potential fix included] #6029

Closed BarbzYHOOL closed 3 years ago

BarbzYHOOL commented 3 years ago

@$variable does not seem to be ignored

I have an issue where a @$variable error is not ignored (using Smarty with CI). The solution from here works flawlessly: https://stackoverflow.com/questions/6610441/error-control-operator-not-working

It's basically adding this bit of code:

// if error has been supressed with an @
if (error_reporting() == 0)
{
    return;
}

Because if the error has been suppressed, it returns 0.

I'm on an old codeigniter 3.0 (but I'll update it soon). The custom error handler is "_error_handler()" on new CI, and on old Ci it's "_exception_handler()".

So I checked the latest file for both function https://github.com/bcit-ci/CodeIgniter/blob/develop/system/core/Common.php

Why not add the code above in one or both the handlers?

PHP 8+

But while writing this, I also found this on the php doc:

Warning Prior to PHP 8.0.0, the value of the severity passed to the custom error handler was always 0 if the diagnostic was suppressed. This is no longer the case as of PHP 8.0.0.

So maybe, this bit of code in Common.php is already handling it?

if (($severity & error_reporting()) !== $severity)
{
    return;
}

If so, then what I said in the beginning is only useful for people using outdated CI 3 (we probably aren't many :D). Else we can maybe check if ! error_reporting() too.

Thank you if you answer me!

gxgpet commented 3 years ago

So...

  1. Yes, upgrading the framework is always a recommended action. :P
  2. The error handler and exception handler, both existed since CI 3.0rc, and they do different actions for different contexts. I'm not sure why are you calling them new and old.
  3. error_reporting() indeed is not returning 0 anymore on PHP 8, if @ is used. And indeed, ($severity & error_reporting()) !== $severity conditions works on PHP 8 as well from a quick test I just performed.
  4. Exceptions can't be suppressed by the @ operator.
BarbzYHOOL commented 3 years ago

On my old version, ../system/core/CodeIgniter.php:70: set_error_handler('_exception_handler'); so it's all done in the exception handler

What I added fixed my issue, just wanted to propose it, if you think it's useless or detrimental to the current CI, then ok

gxgpet commented 3 years ago

Alright... I can't possibly guess what CI 3 version you have, but I can tell you that in CI 3.0rc, which is a really early CI 3 version, in CodeIgniter.php there's a call for both set_exception_handler() and set_error_handler().

I wasn't saying that your proposal is useless, I was just saying that the current code version should properly handle the scenario described by you. That's why upgrading to the latest CI 3 should be a decision that everyone should take. Besides the obvious benefits from this, you can also help us finding and mitigating any real and potential PHP 8 issues. If you will follow this advice and if you find any problems with the actual error handling, please provide in the future a code snippet as well, that can help us reproduce the issue.

BarbzYHOOL commented 3 years ago

I can't update for now, it's too much work for me at the moment (in case it breaks my stuff).

I can't either provide an easy reproducible scenario, and it's related to Smarty (an external template engine https://www.smarty.net/ ). Basically, if I recall right (hope it will make sense), when I was using the Smarty modifier $my_var|default:"foobar", which sets a default value to a variable if the variable is null/empty, Smarty was calling this function:

function smarty_modifiercompiler_default ($params, $compiler)
{
    $output = $params[0];
    if (!isset($params[1])) {
        $params[1] = "''";
    }

    array_shift($params);
    foreach ($params as $param) {
        $output = '(($tmp = @' . $output . ')===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
    }
    return $output;
}

the line $output = was the source of a problem: it logged "variable undefined" in CI logs everytime I made use of that basic Smarty function. I checked on Smarty forums, the developers said it was caused by the custom error handler, so I found CI's one and I made this fix. No more filling the logs with that. Then I quickly compared the master file with mine and posted here in case it solved a rare issue, but maybe it's only on an old CI. Hope it helps

gxgpet commented 3 years ago

I can't either provide an easy reproducible scenario, and it's related to Smarty

Well, this is a magical line that tells us the issue is not related to the CI 3 framework itself, but rather to an external library. And this is out of our control. :P

However, what can I tell you for sure is that the following line won't generate any error at all, regardless of its type, in PHP 8, under both production and development env settings, in the latest CI 3 version:

$variable = @$a_not_set_variable;

I checked on Smarty forums, the developers said it was caused by the custom error handler

While I have no idea what's Smarty's intention, before blaming a custom error handler, by design, that piece of code is wrongly built. There is a dedicated language construct in PHP for checking whether a variable is set or not. Sure, from a functionality point of view it should do its job in the current form, but with a known trade: it could pop a custom error handler installed.

BarbzYHOOL commented 3 years ago

I tried $variable = @$a_not_set_variable; and I got this: ERROR - 2021-04-18 20:45:41 --> Severity: Notice --> Undefined variable: a_not_set_variable. So on my old CI, it is an issue (and not from Smarty).

So if you say it's not happening on recent CI, then you were very right to close this and I'm glad I will just have to trust the updates blindly :P