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

Type of global constant from define() #2644

Closed
sapphirecat opened this issue Jan 16, 2020 · 6 comments
Closed

Type of global constant from define() #2644

sapphirecat opened this issue Jan 16, 2020 · 6 comments

Comments

@sapphirecat
Copy link

I have a Slim Framework app I'm adding Psalm to. There is a global constant defined at runtime during configuration:

<?php
// in the main settings file (slightly simplified),
// included by the front controller
// (phpunit bootstrap file defines it as 'test')
if (! defined('ENVIRONMENT')) {
    define('ENVIRONMENT', $_SERVER['APP_ENV'] ?? 'prod'); 
}

// in a PSR-4 autoloaded class file
class ErrorHandler {
  private function formatException(Throwable $ex) {
    // ...
    if (ENVIRONMENT === 'dev' || ENVIRONMENT === 'test') {
      // add stack trace to error responses
    }
  }
}

This constant is not available when Psalm is running, because it doesn't load in via the framework's front controller. If I give Psalm a bootstrap file that defines the constant, then Psalm ends up thinking the constant can have only one value, and flags the other values as impossible. (Psalm is set up to analyze this settings file, since it's under src/ with everything else, but it doesn't seem to be picking up this information about the constant and applying it to other files. Which is reasonable, since Psalm doesn't know the entry points / load order.)

Maybe the type could be set in <globals> in the config, except that it's not a variable, so that didn't work. It doesn't look like globals supports a const/constant tag.

It seems to be too complex to reproduce on https://psalm.dev – if everything is in one file, then Psalm can recognize that the constant can have multiple values and reports "no issues".

How do I get the correct type, or even just string, to Psalm; or, should I replace the constant with a global variable?

vimeo/psalm 3.8.2.

@weirdan
Copy link
Collaborator

weirdan commented Jan 16, 2020

Does psalm stop complaining if you remove the !defined() check? If it does, I would probably leave the constant definition unconditional (but still dynamic) and instead changed the $_SERVER['APP_ENV'] with <php><server name="APP_ENV" value="test"/></php> stanza in phpunit.xml (Reference).

@sapphirecat
Copy link
Author

sapphirecat commented Jan 16, 2020

Psalm continues to give errors about the undefined constant.

  • src is a PSR-4 autoload root with all the classes I want to analyze
  • public/index.php uses $settings = (include '../src/Config/settings.php'); to get the framework configuration
  • Because some settings depend on ENVIRONMENT, settings.php defines it as a side effect.
  • If I put autoload="src/Config/settings.php" or a custom file that only defines ENVIRONMENT into the config, then psalm treats the constant as always 'prod', and never 'dev' nor 'test'.

I changed <projectFiles> to add public in addition to src, but it also hasn't changed anything. I suspect that psalm doesn't know when settings.php gets read, and assumes that classes in src could be used without the definition in place.

<?xml version="1.0"?>
<psalm
		totallyTyped="false"
		resolveFromConfigFile="true"
		xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
		xmlns="https://getpsalm.org/schema/config"
		xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
>
	<projectFiles>
		<directory name="public"/>
		<directory name="src"/>
		<ignoreFiles>
			<directory name="vendor"/>
		</ignoreFiles>
	</projectFiles>
	<globals>
		<var name="app" type="Slim\App"/>
	</globals>
</psalm>

I'm probably going to use a baseline file and run with --show-info=false, but I'd love to have some way to say <const name="ENVIRONMENT" type="'dev'|'prod'|'test'"/> in the config.

@weirdan
Copy link
Collaborator

weirdan commented Jan 17, 2020

The problem with autoload attribute is that the file it points to is executed - thus all constants acquire their values. What you need instead is to have Psalm read the file before any other to make it see there are other possible values. I've been able to achieve exactly that by referencing the config file as a stub.

I think I was able to reproduce your issue here (omitting Slim dependencies): https://github.com/weirdan/psalm-issue-variable-constant/commits/master. See the commit / build history, this change fixes the issue: https://github.com/weirdan/psalm-issue-variable-constant/commit/92e314964b99bbc31362c2235d2f41bbe11c886c

This feels like a hack to me though. @muglug what do you think? Should there be a config section listing the files to analyze before continuing with those listed in <projectFiles>?

@muglug
Copy link
Collaborator

muglug commented Jan 17, 2020

If I give Psalm a bootstrap file that defines the constant, then Psalm ends up thinking the constant can have only one value, and flags the other values as impossible.

This is the appropriate way to define constants for Psalm, but maybe there should be an extra flag for constants defined using define('FOO', ...) vs those defined using cont FOO = .

I don't want to throw out string values of defined constants entirely, though, as they're often useful.

@muglug muglug closed this as completed in 471d761 Jan 17, 2020
@muglug
Copy link
Collaborator

muglug commented Jan 17, 2020

I fixed the issue that @weirdan isolated – I hope that should be enough.

@sapphirecat
Copy link
Author

sapphirecat commented Jan 17, 2020

I can confirm the <stubs> approach worked for me.

It turned out, there is weirdness that probably only affects me: we actually got this app from a consulting team, who decided to put the config dir below src. The stub isn't recognized in there. (IMO, this is a reasonable choice, since executing a stub would break many things. Stubs probably shouldn't be mixed in with the actual sources.)

I moved config back where it belonged, and then the stub was recognized. Everything's good with me. 👍 Thanks for the help!

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

No branches or pull requests

3 participants