#**phpstan module - discussion**

1 messages · Page 1 of 1 (latest)

long bobcat
#

what flags do we want? by default it's a progress bar - but that doesn't play nicely with CI systems .. so we should disable progress bar and enable it to spit out to STDOUT all the files it's scanning

# 
./vendor/bin/phpstan analyse --no-progress --level=6 -vvv app
#

How do we make phpstan show the filesnames as it scans them?

# 
./vendor/bin/phpstan analyse --no-progress --level=6 --memory-limit=-1 app
# 
dagger /app $  ./vendor/bin/phpstan analyse --no-progress --level=6 --memory-limit=-1 --error-format=raw -vvv ./vendor
#

I'm trying to get it to spit the files out, as it scans them .. how can we do this ?

weak lion
#

$args = [
'vendor/bin/phpstan',
'--no-progress',
"--memory-limit=$this->memoryLimit",
"--error-format=$format",
];

    $this->container = $this->container
        ->withExec(args: $args, expect: ReturnType::ANY);
#

pulled from one of my projects

long bobcat
#

i already poosted them above

#

what I'm asking is .. how can we display what files are being scanned? 🙂

weak lion
#

return the std out

#

$this->container
->withNewFile(StaticAnalysisReport::LOG, $this->container->stdout())

# 
    #[DaggerFunction('Return static analysis report')]
    public function report(): string
    {
        $exitCode = $this->container->exitCode();
        $stdout = $this->container->stdout();
        $stderr = $this->container->stderr();

        if ($exitCode !== 0) {
            throw new QueryError(['errors' => [[
                'message' => 'problem(s) found during static analysis',
                'extensions' => [
                    'exitCode' => $exitCode,
                    'stdout' => $stdout,
                    'stderr' => $stderr,
                ]]]]);
        }

        return $stdout;
    }
vocal trellis
long bobcat
#

Aha

vocal trellis
#

Also if you use --debug it overrides --no-progress so you only need one or the other

#

There doesn't seem to be a way to delay showing what files are scanned until it's finished

#

It just, prints them out as it goes

long bobcat
#

@vocal trellis understood, testing, thanks

#

woohoo - works

long bobcat
#

@vocal trellis @weak lion

I'm thinking of making a "Options Builder" for all the phpstan options ..

What ones do we want to support?

So far I have
no progress
level
error-format
debug
config file path

weak lion
#

Is there a config file option?

long bobcat
#

added

vocal trellis
#

Seems easier to discuss if we copy paste this in here:

Options:
  -c, --configuration=CONFIGURATION            Path to project configuration file
  -l, --level=LEVEL                            Level of rule options - the higher the stricter
      --no-progress                            Do not show progress bar, only results
      --debug                                  Show debug information - which file is analysed, do not catch internal errors
  -a, --autoload-file=AUTOLOAD-FILE            Project's additional autoload file path
      --error-format=ERROR-FORMAT              Format in which to print the result of the analysis
  -b, --generate-baseline[=GENERATE-BASELINE]  Path to a file where the baseline should be saved [default: false]
      --allow-empty-baseline                   Do not error out when the generated baseline is empty
      --memory-limit=MEMORY-LIMIT              Memory limit for analysis
      --xdebug                                 Allow running with Xdebug for debugging purposes
      --fix                                    Launch PHPStan Pro
      --watch                                  Launch PHPStan Pro
      --pro                                    Launch PHPStan Pro
      --fail-without-result-cache              Return non-zero exit code when result cache is not used
  -h, --help                                   Display help for the given command. When no command is given display help for the analyse command
  -q, --quiet                                  Do not output any message
  -V, --version                                Display this application version
      --ansi|--no-ansi                         Force (or disable --no-ansi) ANSI output
  -n, --no-interaction                         Do not ask any interactive question
  -v|vv|vvv, --verbose                         Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug
#

I think memory limit is important. We kept hitting snags with phpstan running out of memory.

#

You can set a sensible default of, say '1G' or '2G'. But I think it's safe to say user's needs are going to differ depending on the size of their project and what they're running dagger on.

long bobcat
#

what's this baseline stuff all about? got a link to learn more ?

vocal trellis
#

Let's say you have 5 errors.

#

you generate a baseline of those 5 errors

#

now phpstan passes if you still have those 5 errors

long bobcat
#

ok

vocal trellis
#

but it actually fails and warns you to generate a new baseline if you fix any of those 5

weak lion
#

That's an interesting one; if generating a baseline, you need a way to retrieve the generated file

long bobcat
#

specify baseline file path ?

vocal trellis
#

That's probably a good option for a separate dagger function

weak lion
#

^ that

vocal trellis
#

Like a generateBaseline(): File

long bobcat
#

nope. separate PHP object, with its own baseline options.

Then you can do ->addBaselineOptions($baselineOptions)

for maximum DX I want 1 entrypoint

#

So can we group together all the baseline shit, into a single BaselineOptions object? tell me what I should name the various options (there seems like a few?)

weak lion
#

not possible really; scanning and generating a baseline are two entirely unrelated tasks

long bobcat
#

ohhh

#

I didn't know.

#

Generating a baseline is surely part of the main run?

#

but really is this a CI task at all?

#

Seems like a localhost thing. kind of like composer.lock

weak lion
#

Yeah, more likely to run locally

long bobcat
#

ok we skip for now. mission complete

#

@weak lion do you know of an online tool where we can collaboratively write onto a single PHP class

#

like google docs, but for code

#

no saving .. just a scratchpad

weak lion
#

Well there's jetbrains colab tool in phpstorm

long bobcat
#

don't have that atm

#

in browser thing

long bobcat
weak lion
#

OK, so questions that need to be answered about a phpstan module.

#
  1. Is the user passing us a Directory or a Container?
    1a) If a directory, how should we decide on a runtime container to run php stan in
    1b) If a container, how do we know i) Where the user's code is in the container, ii) If phpstan is installed already iii) Where the phpstan entry point is in relation to the users code.
  2. Should we be installing PHPStan/providing a container for it?
    2a) If we are installing PHPStan (or providing a container with it in) what version should it be?
  3. Output & results:
    3a) How should they be provided back to the user?
    3b) What if the user needs multiple output formats (eg one for display and another for CI tooling integration)
long bobcat
#

Good questions. Have you already seen my module?

#

3 args

#
  1. Php version
  2. Directory object
  3. Path to test on the passed code
#

That'll work every time

#

We don't need a container, static analysis doesn't need a specific runtime (extensions) to be already setup, so we don't need to pass a Container

#

@weak lion

#

If I was to revise this it would be.

Version, Directory, Path, PhpstanOptions

#

Options are what we discussed above. WDYT @vocal trellis @weak lion @grim vale @cedar crater

grim vale
#

In PhpstanOptions, is there the version of phpstan itself? Or is it options argument for phpstan command (flags like --level --configuration --generate-baseline etc)?

vocal trellis
#

@grim vale I think that's what paul means by a Version argument. EDIT: Ignore that, I just looked above and saw it refers to PHP version

In my opinion, PHPStan version is not important unless it's a legacy project:

The library of performed rules on analysed code, and the meaning of rule levels does not change in minor/patch versions.
https://phpstan.org/user-guide/backward-compatibility-promise#checks-performed-on-analysed-code

PHPStan requires PHP >= 7.4. You have to run it in environment with PHP 7.x but the actual code does not have to use PHP 7.x features. (Code written for PHP 5.6 and earlier can run on 7.x mostly unmodified.)
https://phpstan.org/user-guide/getting-started

Getting Started
#

If the PHPStan version is fixed, then it will run the same, local and CI.

#

But even if it's not a fixed version, minors and patches should mostly be fine.

#

Also how necessary is the PHP version?

The module doesn't run the code, it runs PHPStan. If the version of PHPStan is fixed, then so is the container.

#

Also

PHPStan needs a working autoloader to access reflection of the analysed classes. It uses Composer autoloader in the project by looking at vendor/autoload.php from the current working directory.
https://phpstan.org/user-guide/autoloading

So now the module requires vendor/autoload.php, but you can't simply take that from local, otherwise it's wont run identically on CI.

So now the module requires Composer and a composer.json so it can install vendor itself.
So It requires composer.json, from which it can read the PHP Version of the project anyway.

Autoloading
grim vale
#

from which it can read the PHP Version of the project anyway.
The minimum required version of PHP for the project, yes

vocal trellis
#

Testing against every allowed version would be good, but I think PHPStan only cares what your autoloader says, so it all hinges on what Composer installs, which is dictated by the composer.json required by the module.

vocal trellis
long bobcat
# 
In PhpstanOptions, is there the version of phpstan itself? Or is it options argument for phpstan command (flags like --level --configuration --generate-baseline etc)?

The latter. it's like an "argument builder" object

#

@grim vale

#

@vocal trellis if the PHP version necessary? I think so .. for example .. I think it checks what is and isn't part of that PHP version .. and maybe throws errors/issues .. kind of like a linter.

weak lion
#

In that case, we'd also need the extensions as they tend to add functions

long bobcat
#

hmm

#

I'm asking Ondrej right now

#

Will let you know once he replies

vocal trellis
# long bobcat <@1229425915675807797> if the PHP version necessary? I think so .. for example ....

In my opinion, that is the exact reasoning it should not be an argument:

  1. Happy little developer has PHP 8.4 installed locally and is using the new array_find function to simplify code.
  2. Happy little developer runs the cool new Dagger module for PHPStan: "What version should it run? Why 8.4 of course!"
  3. PHPStan is happy on 8.4, everything passes, He pushes it, CI is set up to run on... you guessed it, 8.4!.
  4. Everyone is happy, CI is green, the code goes into their PHP library.
  5. Happy little developer's colleague rebases his branch to main. Everything breaks!
  6. The colleague is running PHP 8.3, but their composer.json says this should be fine! The CI is passing too!
  7. Colleague is confused.
#

The module should ideally test against all versions allowed by the composer.json

#

That way, happy little developer will notice CI fail after using 8.4 functions and realise to update their PHP requirement.

weak lion
#

certainly an ideal would be to test against all allowed versions in composer.json but for most user projects; they'll have a specific version defined to use.

#

for open source, we'd need that option though

vocal trellis
#

In terms of DX for someone wanting to write a good CI pipeline you would also end up with something like this:

phpstan:
  extends: [.dagger]
  stage: static-analysis
  script:
    - dagger call phpstan --php-version=8.1
    - dagger call phpstan --php-version=8.2
    - dagger call phpstan --php-version=8.3
    - dagger call phpstan --php-version=8.4

Which then has to be updated manually every time the PHP requirement changes (which runs the risk of being forgotten)

#

Or you have just the one call:

phpstan:
  extends: [.dagger]
  stage: static-analysis
  script:
    - dagger call phpstan --all-allowed-versions
vocal trellis
long bobcat
#

@vocal trellis I spoke to Ondrej. specify the version in our function calls to phpstan.

#

@vocal trellis yes, you're right. php version will be a CLI argument. It already works like this, and we'll continue to make it work like that too.

long bobcat
#

I'm not in favour of --all-allowed-versions .. not everyone puts the php version inside composer.json, so we can't predict that .. and now brings more complexity into the mix ..

thus we should always rely on user specified input for the PHP version we're testing against.

vocal trellis
#

As the case may be, but I would say it's bad practice not to put it inside composer.json so it sort of depends if you want to accommodate that practice or throw a user-friendly exception for "please specify the supported php versions for your project in your composer.json"

#

I do agree, it adds complexity to writing the module, but it simplifies the use of the module for the user.

#

It's more of a WBN

long bobcat
#

@vocal trellis bad practise doesn't mean people won't do it. In OSS we can't base solutions on that.

weak lion
long bobcat
vocal trellis
#

The debug option I agree with, and it does just override --no-progress without causing issue

weak lion
vocal trellis
#

Also level can probably be given as an integer argument since it can only be 1-10

long bobcat
#

@vocal trellis it's going to end up as a string anyway. so we'll keep it as string, as we never do any integer-based calculations on it. -- so it's a proxy

#

@weak lion why? why can't I just execute this PHP code in the local SDK and have it populate the options for me ?

weak lion
#

Int prevents passing "ten" though

long bobcat
#

ok fine - int it is

weak lion
long bobcat
#

umm

# 
dag()->phpstan()->analyse($phpVersion, $source, $path, $options);

This is our API

weak lion
#

options needs to be a native type or a dagger object

#

you can't pass a php object in

#

-> how would you call that from python SDK?

long bobcat
#

got it

#

Okay, you'll have to teach me about Dagger Objects .. we did discuss that I had a few queries about them ..

Let me commit this to the repo, and then you can maybe give it a spin and make it a Dagger Object

#

Is that cool?

weak lion
#

Might have time this eve; currently debugging a prometheus/nginx/kube issue

long bobcat
#

fun times, Mr Platform Engineer

weak lion
#

No code in the phpstan repo?

#

In any case; I added a prototype for phpstan support to Sai; you might get some ideas from what I've done

long bobcat
#

@weak lion

weak lion
#

?

long bobcat
#

I have the code

#

It is in another repo - I've moved it across to

#

ready for "dagger install"

#

I'm moving that PhpstanOptions class to the repo now .. to be picked up as a "dagger object"

weak lion
#

Will probably need to pick this up in the morning; just about to go to bed

long bobcat
#

pushed

#

sure, no worries. you have write access, but maybe make a PR so I can "see/learn"

#

I've not included it at all in any of the [DaggerModule] main file, yet

weak lion
#

yeah; I'll find some time tomorrow unless @vocal trellis gets on it first

#

busy the rest of the week though

#

giving another dagger talk tomorrow evening

long bobcat
#

where?

#

leeds?

weak lion
#

York

long bobcat
#

👌 tidy

weak lion
long bobcat
#

once I see how you/we move on this phpstan module .. I'll be able to make the Dagger Objects for the rest of the repos I have prepped and ready to go

long bobcat
weak lion
#

Sai is there if you want to study how that works

long bobcat
#

I pushed a new commit, but daggerverse command has old commit hash

#

is it on a cron? I tried "dagger update ....<new hash from github commits>" but it didn't resolve

#

/cc @potent tartan

weak lion
#

What's the hash in your dagger.json?

long bobcat
#

don't worry, Jeremy is helping me

long bobcat
#

I did a workaround, and had the module build the CLI args as a string .. internally inside the independant module .. not from the "calling code" where we need a DaggerObject

but I got it working 😎

image.png
#

scanning vendor DIR on my current project.

# 
return dag()->phpstan()->analyze('8.4', $this->source, './vendor');
#

Inside the module, I hacked (and hard coded) the new PhpstanOptions stuff

$cliOptions = (new PhpstanOptions())->level("5")->memoryLimit('1G')->buildCliCommand();
#

Wider picture for the module ..

image.png
#

So from any PHP project, where you have dagger .. just install the new module like this

# 
dagger install github.com/dagger-php/phpstan@16c21645512d3ab53738184edd1af353523cdf68
#

🙂

#

Then add this dagger function to your project

# 
    #[DaggerFunction]
    #[Doc('Run phpstan')]
    // dagger call phpstan-module stdout
    public function phpstanModule(): Container
    {
        return dag()->phpstan()->analyze('8.4', $this->source, './vendor');
    }
#

and call that from the CLI like this

dagger call phpstan-module stdout
#

💥

#

bim bam boom

#

run this ☝️ and it should "just work" - make sure you have a composer.json in your current project you're adding this to, as the code above wants to scan ./vendor

#

thanks @potent tartan 🤝

vocal trellis
#

In terms of making PhpstanOptions, it depends what you want the dagger call to look like

#

For instance, my first thought is you'd want something like this:

dagger call phpstan-moduleoptions debug level --level=6 phpstan analyse

Or for basic settings:

dagger call phpstan-module analyse
#

In that instance you'd need an options method on Phpstan and a phpstan method on PhpstanOptions

#

The alternative might be to reduce it a bit, something like this:

dagger call phpstan-module options debug level --level=6 analyse

Or for basic settings:

dagger call phpstan-module analyse

But that option would involve both classes needing an analyse method.

#

TLDR; the options are:

  1. shorter code, longer dagger calls
  2. longer code, shorter dagger calls
#

Or any third option if you had something else in mind

vocal trellis
#

The best bet is probably to come up with your ideal dagger call, then we know how to structure it

long bobcat
#

For now we're going with less CLI arguments. The Options can be customised more in the code. This is how most teams are using phpstan in CI contexts, already.

grim vale
vocal trellis
long bobcat
#

What's the benefit ? @grim vale

#

We are using "phpqa"

#

It has all the tools baked in meaning 1 image for all the things.. now we are pulling 2 images.. so the Dagger CI pipeline will be slow

grim vale
#

Because the official PHPStan image already contains Composer, and it is all we need to run PHPStan analyze.
https://github.com/phpstan/phpstan/blob/2.1.x/docker/Dockerfile.php.8.4

In addition, the less we have in the Docker image, the less we are exposed to security issues from tools installed and not used by the PHPStan Dagger module.

I'll be in favor of strictly bounded images for PHP modules, to reduce the security risks by design

vocal trellis
#

Security-wise; smaller surface area is definitely a benefit here, as @grim vale pointed out.
I'm in favour of this too.

Also, separation of concerns; the PHPStan module shouldn't be concerned with how it interacts with a PHPUnit module, or vice versa.

If you need a compromise between the two. I would propose something like this:

#[DaggerFunction]
public function __construct(
    private ?Container $container
) {
    if ($this->container === null) {
        $this->container = dag()
            ->container()
            ->from('phpstan image');
    }
}

This way, if the user simply wants to use the PHPStan module separately, they can, with a nice lean focused container.

Then, if you create some sort of monolithic module, wrangling all CI into one. It can provide it's own container phpqa set up to play nicely with it.

grim vale
#

Or wait for someone to provide or ask for a phpqa module. From my point of view, we first need to let people know about Dagger PHP Modules, the best way IMHO is to provide modules for most popular PHP qa tools (phpstan, php-cs-fixer, phpunit, psalm, pest, pint and rector at least). Then, when users will be there for those basic and super popular modules, we will have feedback through github issues/discussions about what they need/want and the we can provide modules for more tools, or gather some tools in 1 module.

Without initial feedback, I'm afraid we will loop indefinitely to decide how/what to ship in a module. So let's ship straight and basic one, and listen for feedback? 😉

long bobcat
#

I've read everything you guys have said.

We are going to keep using phpqa image because it solves a number of other problems with Dagger itself (stuff you're not aware of, other conversations I'm having) so we really need to focus on speed right now.

Pulling 1 image down for all our quality tooling needs to be the primary driver here. For now. Otherwise more problems are going to occur (I'm looking broader right now, than this discussion, and looking a bit ahead of the dagger roadmap). Hope that makes sense.

Down the line once these other factors go away, then we can consider pulling down multiple images, but right now we're not there yet.

#

Separately, I'm on a call with Chris, just my notes below

# 
        $options = dag()->phpstan()->makeOptions()->level("4");
        return dag()->phpstan()->analyze('8.4', $this->source, './vendor', $options);
#

finished with Chris

long bobcat
#

@weak lion did you sync with Dagger team on what we are looking to do? And how to best achieve it

weak lion
#

Oh, I found what the issue was; the PHP SDK currenly only supports custom objects as a return type not as an argument. Was waiting till Monday to find out from John if there was a reason for that (eg we have a reason for not doing enum support yet) or if it was something we intended to come back to. If the latter; will have a look at how python & typescript handle it and work on a solution from there

long bobcat
#

Okay.

#

@topaz marten @forest mortar

Can you send us github links to how this is done in Python or TS?

Custom Dagger Objects as Arguments into dagger functions.

# 
$options = dag()->phpstan()->makeOptions();

return dag()->phpstan()->analyze('8.4', $this->source, './vendor', $options);
#

Options is a dagger object from our phpstan() module and we pass that to the phpstan() module analyze function

vocal trellis
#

I know what needs to be done to support it, just need the time assigned to the task. However this week is a critical task that cannot be postponed so it wont be this week.

topaz marten
weak lion
#

The Tl;DR is that there was a simple piece of code which was developed for the initial MVP implementation, which was mostly phased out when adding custom objects; however it's still in use for argument decoding. The fix is to implement the full deserialisation for arguments and remove the original code completely.

vocal trellis
#

To add to what @weak lion said: Originally, it may have been missed simply because testing had to be done manually every single time. But Jed (thank you!) has since taught me how to add integration tests for the SDK so I should be able to do the following:

  1. Add passing integration tests for supported argument types
  2. Add failing integration tests for custom object arguments
  3. Replace the original code with confidence
  4. (Hopefully) See Failing tests now pass
long bobcat
#

@weak lion have you seen the example from @topaz marten and do you know how to implement this ?

weak lion
#

Yeah see John's explainer above. We'll pick it up tail end of next week

long bobcat
#

Okay, I'll put down that we are blocked on this module, until we add this.

#

/cc @potent tartan see some edge case feature not yet in our SDK (we didn't come across it yet) .. I tried to make a module and hit something (custom dagger objects, as arguments to a dagger module)

We have a plan to add it

#

@weak lion what about this btw? 😛 to avoid the custom object as an argument

dag()->phpstan()->analyse()->memoryLimit('..')->level(6)->run()

No custom object needed.

Do we like this syntax? /cc @vocal trellis @grim vale

weak lion
#

That's how I've done other things previously

vocal trellis
#

Yeah I think that looks fine 👍

long bobcat
#

well well then. i'll continue with that .. as a stop gap

long bobcat
#

@weak lion @vocal trellis so this doesn't work

on the module itself :/

image.png
#

I tried this

        return dag()->phpstan()
            ->memoryLimit('2G')
            ->analyze('8.4', $this->source, './app');
#

$options is empty.

#

like when I call analyze() it is losing all the previous state

weak lion
#

no return types on the functions; and no attribute denoting a function

#

suprised it ran at all 🤔

#

probably used a cached version as the new one failed to parse properly would be my guess

long bobcat
#

@weak lion I've got it working btw

#

I got rid of arrays, moved to strings

#

This is now the syntax for the module .. when calling it from the application

#

🙂

#

Internally it populates memlimit and level as strings .. and in analyze() I just access them

#

mision complete .. for v1

#

this is the internal module code

makeCliArgs() and a PhpstanArgBuilder class

image.png
long bobcat
#

PHP Version is now an optional function call and not part of the mandatory analyse() call

  #[DaggerFunction]
  #[Doc('Run phpstan on your codebase with some arguments')]
  public function staticWithArgs(
      Directory $source,
      string $phpVersion = '8.4',
      string $pathToTest = '.'
  ): Container
  {
      return dag()
          ->phpstan()
          ->phpVersion($phpVersion)
          ->analyse(source: $source, pathToTest: './app');
  }
vocal trellis
#

You might prefer having the $phpVersion in a constructor:

  #[DaggerFunction]
    public function __construct(
        private string $phpVersion = '8.4',
    ) {}

Then the php code looks like this:

      return dag()
          ->phpstan($phpVersion)
          ->analyse(source: $source, pathToTest: './app');