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

$_SERVER['APP_DEBUG'] is considered as a string instead of a bool. #8556

Closed
VincentLanglet opened this issue Oct 9, 2022 · 13 comments · Fixed by #8561
Closed

$_SERVER['APP_DEBUG'] is considered as a string instead of a bool. #8556

VincentLanglet opened this issue Oct 9, 2022 · 13 comments · Fixed by #8561

Comments

@VincentLanglet
Copy link
Contributor

Hi @orklah @kkmuffme , I have a similar issue

After the merge and the release of #8473, I now have with the code

$kernel = new AppKernel($_SERVER['APP_ENV'], $_SERVER['APP_DEBUG']);

the error

InvalidArgument: Argument 2 of Sonata\UserBundle\Tests\App\AppKernel::__construct expects bool, but string provided

I have

    <php>
        <ini name="precision" value="8" />
        <env name="SYMFONY_DEPRECATIONS_HELPER" value="max[self]=0" />
        <env name="KERNEL_CLASS" value="\Sonata\UserBundle\Tests\App\AppKernel" />
        <server name="APP_ENV" value="test" force="true" />
        <server name="APP_DEBUG" value="false" />
    </php>

in my phpunit.xml.dist config, and when I dump

$_SERVER['APP_DEBUG']

this is correctly the bool false and not a string.

There might be some discussion about Request_time_float (cf #8552) but, here, in APP_DEBUG, I don't feel like I put a wrong value.

@psalm-github-bot
Copy link

Hey @VincentLanglet, can you reproduce the issue on https://psalm.dev ?

@orklah
Copy link
Collaborator

orklah commented Oct 9, 2022

Seems different than the float one if PHP itself outputs a boolean

@kkmuffme this seems legit, don't you think?

Now the question is if that can be string or if it's always bool

@kkmuffme
Copy link
Contributor

This is expected, as I couldn't plan for all possible cases, I just added was is declared in the specification and commonly found in nginx/apache/cloudflare.

$_SERVER is an array containing information such as headers, paths, and script locations. The entries in this array are created by the web server. There is no guarantee that every web server will provide any of these; servers may omit some, or provide others not listed here

Technically parsing XML into $_SERVER isn't what is intended to be done in PHP though, as $_SERVER should only be populated by the actual webserver.
I think however, for common PHP frameworks we can make an exception and add them to the list, once they're reported here (like this issue) - @orklah what do you think?
Generally though, you can set you own $_SERVER superglobal as you need it via psalm's config file (which will be the way to go for more obscure frameworks when people don't want to use @psalm-suppress)

I will provide the PR for it today for APP_ENV non-empty-string and APP_DEBUG bool if that's fine.

@VincentLanglet
Copy link
Contributor Author

I think however, for common PHP frameworks we can make an exception and add them to the list, once they're reported here (like this issue) - @orklah what do you think?
Generally though, you can set you own $_SERVER superglobal as you need it via psalm's config file (which will be the way to go for more obscure frameworks when people don't want to use @psalm-suppress)

I will provide the PR for it today for APP_ENV non-empty-string and APP_DEBUG bool if that's fine.

I dunno how sever global are usually used for Symfony framework.

For instance, there is

new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);

in the Symfony code. And I found https://blog.digital-craftsman.de/beware-of-app-debug-false/

So I wonder if it's not phpunit which is casting the APP_DEBUG=false value automatically from the config.

I just found out the "issue" is related to phpunit which is doing

private function getBoolean(string $value, $default)
    {
        if (strtolower($value) === 'false') {
            return false;
        }

        if (strtolower($value) === 'true') {
            return true;
        }

        return $default;
    }

on every 'var', 'env', 'post', 'get', 'cookie', 'server', 'files', 'request' values.

I'm not sure what is the best strategy between

  • Changing APP_DEBUG to string|bool
  • Doing nothing (people should cast by themself)

@kkmuffme
Copy link
Contributor

Yes, phpunit does it and populates the $_SERVER config, which it shouldn't. But alas, since phpunit is common, I will add it for now as bool

@VincentLanglet
Copy link
Contributor Author

Yes, phpunit does it and populates the $_SERVER config, which it shouldn't. But alas, since phpunit is common, I will add it for now as bool

if you use 1 or 0 as value in the phpconfig or in the .env config, the value will be a string.
That's why I start to think you made the right call to consider those values as string.

It can prevent some error from people writing

APP_DEBUG='false'

in the .env file. And it force to cast for people writing

APP_DEBUG='0'

That's why if we add APP_DEBUG, it should be string|bool and not just bool.
it can also stay string, and we consider as a good practice to cast to bool.

I personally will add (bool) when I use this value.

@kkmuffme
Copy link
Contributor

I think it's fine to enforce bool here, when we make it explicit now anyway, might as well force people to use true/false in the config file, if that's what is expected here.

@VincentLanglet
Copy link
Contributor Author

if that's what is expected here.

It's expected when you're setting them with phpunit, but not if you set them with a .env.test file which is also a way to do it.
That's why I said it's not always a bool finally.

@kkmuffme
Copy link
Contributor

Ok changed it in the PR to bool|string

@VincentLanglet
Copy link
Contributor Author

@orklah @kkmuffme Now I get the following errors

tests/custom_bootstrap.php:20:25: PossiblyUndefinedArrayOffset: Possibly undefined array key $_SERVER['APP_ENV'] on array{APP_DEBUG?: bool|string, APP_ENV?: string, AUTH_TYPE?: non-empty-string, CONTENT_LENGTH?: string, CONTENT_TYPE?: string, DOCUMENT_ROOT?: non-empty-string, FCGI_ROLE?: non-empty-string, GATEWAY_INTERFACE?: non-empty-string, HOME?: non-empty-string, HTTPS?: string, HTTP_ACCEPT?: non-empty-string, HTTP_ACCEPT_CHARSET?: non-empty-string, HTTP_ACCEPT_ENCODING?: non-empty-string, HTTP_ACCEPT_LANGUAGE?: non-empty-string, HTTP_CACHE_CONTROL?: non-empty-string, HTTP_CDN_LOOP?: non-empty-string, HTTP_CF_CONNECTING_IP?: non-empty-string, HTTP_CF_IPCOUNTRY?: non-empty-string, HTTP_CF_VISITOR?: non-empty-string, HTTP_CLIENT_IP?: non-empty-string, HTTP_CONNECTION?: non-empty-string, HTTP_COOKIE?: non-empty-string, HTTP_DNT?: non-empty-string, HTTP_HOST?: non-empty-string, HTTP_PRIORITY?: non-empty-string, HTTP_REFERER?: non-empty-string, HTTP_SEC_CH_UA?: non-empty-string, HTTP_SEC_CH_UA_MOBILE?: non-empty-string, HTTP_SEC_CH_UA_PLATFORM?: non-empty-string, HTTP_SEC_FETCH_DEST?: non-empty-string, HTTP_SEC_FETCH_MODE?: non-empty-string, HTTP_SEC_FETCH_SITE?: non-empty-string, HTTP_SEC_FETCH_USER?: non-empty-string, HTTP_UPGRADE_INSECURE_REQUESTS?: non-empty-string, HTTP_USER_AGENT?: non-empty-string, HTTP_X_FORWARDED_FOR?: non-empty-string, HTTP_X_FORWARDED_PROTO?: non-empty-string, HTTP_X_REAL_IP?: non-empty-string, ORIG_PATH_INFO?: non-empty-string, PATH?: non-empty-string, PATH_INFO?: non-empty-string, PATH_TRANSLATED?: non-empty-string, PHP_AUTH_DIGEST?: non-empty-string, PHP_AUTH_PW?: non-empty-string, PHP_AUTH_USER?: non-empty-string, PHP_SELF?: non-empty-string, QUERY_STRING?: string, REDIRECT_REMOTE_USER?: non-empty-string, REDIRECT_STATUS?: non-empty-string, REMOTE_ADDR?: non-empty-string, REMOTE_HOST?: non-empty-string, REMOTE_PORT?: string, REMOTE_USER?: non-empty-string, REQUEST_METHOD?: non-empty-string, REQUEST_SCHEME?: non-empty-string, REQUEST_TIME?: int<1665675034, max>, REQUEST_TIME_FLOAT?: float, REQUEST_URI?: non-empty-string, SCRIPT_FILENAME?: non-empty-string, SCRIPT_NAME?: non-empty-string, SERVER_ADDR?: non-empty-string, SERVER_ADMIN?: non-empty-string, SERVER_NAME?: non-empty-string, SERVER_PORT?: non-empty-string, SERVER_PROTOCOL?: non-empty-string, SERVER_SIGNATURE?: non-empty-string, SERVER_SOFTWARE?: non-empty-string, USER?: non-empty-string, argc?: int<1, max>, argv?: non-empty-list<string>}<non-empty-string, string> (see https://psalm.dev/167)

This is kinda annoying since the APP_ENV and APP_DEBUG values will always be defined for most of phpunit/symfony users.

Is there something we can do about it ?

@orklah
Copy link
Collaborator

orklah commented Oct 13, 2022

Do you use the "ensureArrayStringOffsetsExist" config?

@VincentLanglet
Copy link
Contributor Author

Do you use the "ensureArrayStringOffsetsExist" config?

No.
Might be because of https://github.com/vimeo/psalm/pull/8561/files#diff-e7ee5af78a835688eb9916b6dc71f36801f67131a3cb2d8d7604fe1586d2c169R651

@kkmuffme
Copy link
Contributor

This is correct and not a bug. While it might exist for you, it might not exist generally in all environments.
If you are sure this exists in your environment, then please set it in your psalm config file for the superglobal.

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 a pull request may close this issue.

3 participants