phpstan / phpstan-symfony

Symfony extension for PHPStan
MIT License
696 stars 89 forks source link

any way to support typing for $request->request->get()? #262

Open arderyp opened 2 years ago

arderyp commented 2 years ago

right now I'm having to check the type on each call (mixed return), which is a lot. Not sure if this is possible, but thought I'd ask.

ondrejmirtes commented 2 years ago

Can you be more specific with a code sample and the error you're getting?

arderyp commented 2 years ago

I see there is $request->request->getInt(), which is helpful in reducing some excessive type checking code.

ondrejmirtes commented 2 years ago

Still not sure what this is about. I'll reopen if you clarify, thanks.

arderyp commented 2 years ago
$commentNew = $request->request->get('value');
$message = "Updated comment: $commentNew";

PHPSTAN ERROR: phpstan: Part $commentNew (mixed) of encapsed string cannot be cast to string.

arderyp commented 2 years ago

hopefully the above clarifies @ondrejmirtes

ondrejmirtes commented 2 years ago

Can you please post these? Put them in the source code and re-run PHPStan.

\PHPStan\dumpType($request);
\PHPStan\dumpType($request->request);
\PHPStan\dumpType($request->request->get('value'));
arderyp commented 2 years ago

I'm getting an unknown function error. I tried explicitly including the function use function PHPStan\dumpType; and that didn't help. I see the function in my vendor directory, but for what it is worth, its just an empty function:

<?php

declare (strict_types=1);
namespace PHPStan;

/**
 * @param mixed $value
 */
function dumpType($value) : void
{
}
arderyp commented 2 years ago

Oops, maybe the result I am looking for is in PHPStan, not the browser:

$ ./bin/phpstan
Note: Using configuration file /path/to/project/phpstan.neon.
 445/445 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------------------------------------- 
  Line   src/App/Controller/MyController.php                                 
 ------ ------------------------------------------------------------------------- 
  107    Part $searchString (mixed) of encapsed string cannot be cast to string.  
  109    Dumped type: Symfony\Component\HttpFoundation\Request                    
  110    Dumped type: Symfony\Component\HttpFoundation\ParameterBag               
  111    Dumped type: mixed                                                                         
 ------ ------------------------------------------------------------------------- 
arderyp commented 2 years ago

These are all results I expect, as ParameterBag::get() returns mixed.

I've proposed a solution to Symfony that could be helpful if accepted: https://github.com/symfony/symfony/issues/45775

ondrejmirtes commented 2 years ago

This is not the result I expect, do you have the latest version of PHPStan and phpstan-symfony? Is the extension enabled?

arderyp commented 2 years ago

Well, that's good news! But I am afraid I do have the latest phpstan (1.4.10) and phpstan-symfony (1.1.7). I am using phpstan/extension-installer (1.1.0), which I believe enables the extensions automatically. And my phpstan.neon includes:

symfony:
  ## @todo SYMFONY 5.4 this will need to change, see: https://github.com/phpstan/phpstan-symfony
  containerXmlPath: var/cache/dev/srcDevDebugContainer.xml
  consoleApplicationLoader: tests/console-application.php

tests/console-application.php is:

<?php

declare(strict_types=1);

// @todo Symfony 5.4 update this file, see https://github.com/phpstan/phpstan-symfony

use App\Kernel;
use Symfony\Bundle\FrameworkBundle\Console\Application;

require __DIR__ . '/../config/bootstrap.php';
$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);
return new Application($kernel);
arderyp commented 2 years ago

I'm looking at the symfony 5 config for phpstan-symfony:

use App\Kernel;
use Symfony\Bundle\FrameworkBundle\Console\Application;
use Symfony\Component\Dotenv\Dotenv;

require __DIR__ . '/../vendor/autoload.php';

(new Dotenv())->bootEnv(__DIR__ . '/../.env');

$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);
return new Application($kernel);

While I'm running 4.4, I do use flex, I do have vendor/autoload.php, and I am using .env... should I be using this file instead?

arderyp commented 2 years ago

looks like bootEnv() is not available on Symfony 4.4's Dotenv

arderyp commented 2 years ago

this did not change the output of the dumpType calls:

use App\Kernel;
use Symfony\Bundle\FrameworkBundle\Console\Application;
use Symfony\Component\Dotenv\Dotenv;

require __DIR__ . '/../config/bootstrap.php';
require __DIR__ . '/../vendor/autoload.php';

(new Dotenv())->loadEnv(__DIR__ . '/../.env');

$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);
return new Application($kernel);
arderyp commented 2 years ago

Found something potentially interesting. ParameterBagInterface shows return type of @return array|bool|string|int|float|\UnitEnum|null. But the Symfony\Component\HttpFoundation\ParameterBag does not extend this interface (like \Symfony\Component\DependencyInjection\ParameterBag does). Instead, the HttpFoundation bag hints: @return mixed. It's the HttpFoundation bag that's used for $request->request and $request->query.

I see a stub for HttpFoundation bag here: https://github.com/phpstan/phpstan-symfony/blob/master/stubs/Symfony/Component/HttpFoundation/ParameterBag.stub

But most other instances of ParameterBag referenced in phpstan-symfony are the DependencyInjection bag, including in ServiceDynamicReturnTypeExtension here: https://github.com/phpstan/phpstan-symfony/blob/master/src/Type/Symfony/ServiceDynamicReturnTypeExtension.php#L18

ondrejmirtes commented 2 years ago

This is the problem:


 ------ ------------------------------------------------------------------------- 
  Line   src/App/Controller/MyController.php                                 
 ------ ------------------------------------------------------------------------- 
  107    Part $searchString (mixed) of encapsed string cannot be cast to string.  
  109    Dumped type: Symfony\Component\HttpFoundation\Request                    
  110    Dumped type: Symfony\Component\HttpFoundation\ParameterBag               
  111    Dumped type: mixed                                                                         
 ------ ------------------------------------------------------------------------- 

I'd expect this instead:

  110    Dumped type: Symfony\Component\HttpFoundation\InputBag<string|int|float|bool>      
  111    Dumped type: string|int|float|bool                                                                                             

Because: https://github.com/phpstan/phpstan-symfony/blob/79a26b97a85af36367fc90fe2cef481e66c27bf3/stubs/Symfony/Component/HttpFoundation/Request.stub#L8-L13

ondrejmirtes commented 2 years ago

Not sure how it can be happening if everything's properly installed.

arderyp commented 2 years ago

Hmm. @ondrejmirtes Do you suggest I manually install phpstan-symfony? Not sure how to proceed with testing...

arderyp commented 2 years ago

@ondrejmirtes, any input on my previous comment? I am not fully competent on how stubbing works, but is it really possible to type hint a completely different object? Request defines:

/**
     * Request body parameters ($_POST).
     *
     * @var ParameterBag
     */
    public $request;

That's why I was not surprised to see the dumped variable as an instance of ParameterBag instead of InputBag. I'm on Symfony 4.4, if that matters.

ondrejmirtes commented 2 years ago

Please create a small reproducing repository so I can check out the problem.

arderyp commented 2 years ago

If I find some time I will look into that and comment back here.

arderyp commented 2 years ago

well, that was easier than I thought: https://github.com/arderyp/phpstan-issue-262

@ondrejmirtes

arderyp commented 2 years ago

should we open this back up @ondrejmirtes?

ondrejmirtes commented 2 years ago

I think this code doesn't execute for you: https://github.com/phpstan/phpstan-symfony/blob/1.2.x/src/Symfony/InputBagStubFilesExtension.php Can you try installing this package? https://packagist.org/packages/symfony/http-foundation

arderyp commented 2 years ago
$ composer require symfony/http-foundation
Info from https://repo.packagist.org: #StandWithUkraine
./composer.json has been updated
Running composer update symfony/http-foundation
Loading composer repositories with package information
Info from https://repo.packagist.org: #StandWithUkraine
Updating dependencies
Nothing to modify in lock file
Writing lock file
Installing dependencies from lock file (including require-dev)
Nothing to install, update or remove
Generating autoload files
35 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
phpstan/extension-installer: Extensions installed
> phpstan/phpstan-symfony: installed

Run composer recipes at any time to see the status of your Symfony recipes.

Executing script cache:clear [OK]
Executing script assets:install public [OK]

user@laptop ~/test/phpstan-issue-262 {main*}
$ git diff composer.json
diff --git a/composer.json b/composer.json
index c4fcbd7..a58abdf 100644
--- a/composer.json
+++ b/composer.json
@@ -10,6 +10,7 @@
         "symfony/dotenv": "4.4.*",
         "symfony/flex": "^1.3.1",
         "symfony/framework-bundle": "4.4.*",
+        "symfony/http-foundation": "4.4.*",
         "symfony/yaml": "4.4.*"
     },
     "require-dev": {

user@laptop ~/test/phpstan-issue-262 {main*}
$ ./vendor/bin/phpstan
Note: Using configuration file /home/user/test/phpstan-issue-262/phpstan.neon.
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ----------------------------------------------------------------------- 
  Line   Controller/DemoController.php                                          
 ------ ----------------------------------------------------------------------- 
  18     Part $commentNew (mixed) of encapsed string cannot be cast to string.  
  20     Dumped type: Symfony\Component\HttpFoundation\Request                  
  21     Dumped type: Symfony\Component\HttpFoundation\ParameterBag             
  22     Dumped type: mixed                                                     
 ------ ----------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------- 
  Line   Kernel.php                                                                                       
 ------ ------------------------------------------------------------------------------------------------- 
  23     Generator expects value type Symfony\Component\HttpKernel\Bundle\BundleInterface, object given.  
 ------ ------------------------------------------------------------------------------------------------- 

 [ERROR] Found 5 errors

commit.

So, unfortunately, http-foundation doesn't appear to resolve the issue. As I suspected, it is already included in symfony/framework-bundle, so installing it explicitly did not change my vendor setup at all (see composer output above).

I should note, I did not build some unique custom symfony environment to reproduce this issue in my test repo. I simply followed the symfony documentation for generating a new project and new controller using the symfony command. Again, it may be worth noting, I generated for symfony version 4.4, since that is what I am running in my other project where this issue surfaced.

Since this issue is affecting an out of the box installation, shouldn't we re-open it?

arderyp commented 2 years ago

Hey @ondrejmirtes, I believe part of the confusions here is stemming from the fact that you were expecting an object of type Symfony\Component\HttpFoundation\InputBag, but it looks like that wasn't introduced until Symfony 5.1: https://github.com/symfony/http-foundation/blob/5.4/CHANGELOG.md#510

the 5.4 InputBag type hints: @return string|int|float|bool|null

the 4.4 ParameterBag type hints: @return mixed

I am running Symfony 4.4, so maybe this is just the expected/limited behavior of the less-type-friendly 4.4 version of Symfony, which uses ParameterBag everywhere.

ondrejmirtes commented 2 years ago

So upgrade your Symfony version - you'll save yourself more trouble that way :)

arderyp commented 2 years ago

I plan to this summer, but it's a major version upgrade, not a simple composer update and call it a day.

Glad the newer versions play more nicely with typing and phpstan though

TomBrunner commented 2 years ago

Hi, I might be misunderstanding the use case of this extension, for the rule

'Provides correct return type for InputBag::get() method based on the $default parameter.'

I would expect \PHPStan\dumpType($request->request->get('start_date', 'test')); to print Dumped type: string, but it still shows as Dumped type: bool|float|int|string|null. Is this not what is supposed to happen?

arderyp commented 2 years ago

Good question @TomBrunner

I haven't heard back from maintainers on my suggested improvements for typed getters on the *Bag objects: https://github.com/symfony/symfony/issues/45775