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

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

Open
arderyp opened this issue Mar 16, 2022 · 30 comments
Open

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

arderyp opened this issue Mar 16, 2022 · 30 comments

Comments

@arderyp
Copy link

arderyp commented Mar 16, 2022

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
Copy link
Member

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

@arderyp
Copy link
Author

arderyp commented Mar 16, 2022

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

@ondrejmirtes
Copy link
Member

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

@arderyp
Copy link
Author

arderyp commented Mar 16, 2022

$commentNew = $request->request->get('value');
$message = "Updated comment: $commentNew";

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

@arderyp
Copy link
Author

arderyp commented Mar 16, 2022

hopefully the above clarifies @ondrejmirtes

@ondrejmirtes
Copy link
Member

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
Copy link
Author

arderyp commented Mar 17, 2022

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
Copy link
Author

arderyp commented Mar 18, 2022

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
Copy link
Author

arderyp commented Mar 18, 2022

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

I've proposed a solution to Symfony that could be helpful if accepted: symfony/symfony#45775

@ondrejmirtes
Copy link
Member

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

@arderyp
Copy link
Author

arderyp commented Mar 18, 2022

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
Copy link
Author

arderyp commented Mar 18, 2022

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
Copy link
Author

arderyp commented Mar 18, 2022

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

@arderyp
Copy link
Author

arderyp commented Mar 18, 2022

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
Copy link
Author

arderyp commented Mar 18, 2022

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
Copy link
Member

ondrejmirtes commented Mar 21, 2022

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:

/**
* Request body parameters ($_POST).
*
* @var InputBag<string|int|float|bool>
*/
public $request;

@ondrejmirtes
Copy link
Member

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

@arderyp
Copy link
Author

arderyp commented Mar 25, 2022

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

@arderyp
Copy link
Author

arderyp commented Mar 31, 2022

@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
Copy link
Member

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

@arderyp
Copy link
Author

arderyp commented Mar 31, 2022

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

@arderyp
Copy link
Author

arderyp commented Mar 31, 2022

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

@ondrejmirtes

@arderyp
Copy link
Author

arderyp commented Apr 5, 2022

should we open this back up @ondrejmirtes?

@ondrejmirtes
Copy link
Member

@arderyp
Copy link
Author

arderyp commented Apr 6, 2022

$ 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?

@ondrejmirtes ondrejmirtes reopened this Apr 7, 2022
@arderyp
Copy link
Author

arderyp commented Apr 13, 2022

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
Copy link
Member

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

@arderyp
Copy link
Author

arderyp commented Apr 17, 2022

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
Copy link

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
Copy link
Author

arderyp commented Apr 20, 2022

Good question @TomBrunner

I haven't heard back from maintainers on my suggested improvements for typed getters on the *Bag objects: symfony/symfony#45775

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