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

Fix deprecated message appearing in web context with PHP 8.1 #64

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

driehle
Copy link
Contributor

@driehle driehle commented Jul 28, 2022

Using krumo with PHP 8.1 and strict types always gives a deprected message:

Deprecated: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in [...]/vendor/mmucklo/krumo/class.krumo.php on line 503

It appears that the @$d['class'] is problematic here, which results in null if not defined:

if (in_array($function, array("krumo","k","kd")) || (strToLower(@$d['class']) == 'krumo') || (is_callable($callback) && call_user_func($callback, $d))) {
    break;
}

This PR fixes the issue by checking the existence with isset($d['class']) first.

Would be happy, if @mmucklo could release a fixed version! :-)

@scottchiefbaker
Copy link
Collaborator

scottchiefbaker commented Jul 31, 2022

What triggers this error on PHP 8.1?

The fix seems innocuous enough, but I think we should document what's triggering the error. I do not, get any errors of this type on PHP 8.0.

@driehle
Copy link
Contributor Author

driehle commented Jul 31, 2022

The error only occurs with PHP 8.1 and strict types enabled. Furthermore, as the relevant code fragment is in the section of peparing the backtrace for web, the issue only occurs when using Krumo in a non-CLI SAPI. It does not happen in CLI (but there is another, different deprecation warning, I just found out...).

The following code is enough. Note that you need to call Krumo from within a function, as otherwise - if called from the main context - there will be no backtrace being iterated in line 503.

<?php

declare(strict_types=1);

require_once 'vendor/autoload.php';

$myFn = function() {
    Krumo::dump(null);
};

$myFn();

You can use the build-in webserver as follows to have a non-CLI SAPI:

php8.1 -S 0.0.0.0:8080 -d display_errors=on -d error_reporting=E_ALL test.php

This is what the error looks like when accessing http://localhost:8080/:

grafik

@driehle
Copy link
Contributor Author

driehle commented Jul 31, 2022

... comment about trim()...

➡️ Moved into #65

@scottchiefbaker
Copy link
Collaborator

scottchiefbaker commented Jul 31, 2022

The trim() should go in a separate PR. Probably makes more sense to do:

$out = var_export($i) ?? "";

Instead of casting to a string? I dunno, what do you think? If you get these two issues separated I'll work on landing them ASAP.

@driehle driehle changed the title Fix deprecated message appearing with PHP 8.1 Fix deprecated message appearing in web context with PHP 8.1 Aug 1, 2022
@driehle
Copy link
Contributor Author

driehle commented Aug 1, 2022

I have updated this PR accordingly and created a separate PR for the CLI issue.

@scottchiefbaker
Copy link
Collaborator

Per #65 we're going to bump the required PHP version to 7.0 which will give us null coalesce. If we do that I think we should modify this fix ever to use NC.

Line 503 should be something like:

$class = strtolower($d['class'] ?? "");

And the if should be changed to check the new $class variable. I think that will be cleaner and easier to read. What do you think?

@driehle
Copy link
Contributor Author

driehle commented Aug 1, 2022

Done

@scottchiefbaker
Copy link
Collaborator

Excellent!

@scottchiefbaker scottchiefbaker merged commit c2972cc into mmucklo:master Aug 1, 2022
@scottchiefbaker
Copy link
Collaborator

@driehle I have updated the next branch with the two fixes we just landed, and bumped the version 0.7.0. Are you able to test with that branch to make sure we're solid with PHP 8.1?

I don't have PHP 8.1 easily accessible to test with. If you test with it for a couple of days and things are solid I'll look at doing a new release here by the end of the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants