PHPStan Fixes

[MGatner]

Description

• Adds composer.json to the path list for PHPStan Action
• Specifies the Same Site Cookie string values for PHPStan compliance
• Removes the ignore line for a now-passing check

See #4134

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• n/a User guide updated
• Conforms to style guide

[MGatner]

It appears that Rector does not understand explicit strings for PHPDoc types, which PHPStan requires. @samsonasik or @paulbalandan have any suggestions?

[MGatner]

E.g.

2) app/Config/Security.php
28 
29     ---------- begin diff ----------
30 --- Original
31 +++ New
32 @@ -86,7 +86,7 @@
33  	 * Defaults to `Lax` as recommended in this link:
34  	 * @see https://portswigger.net/web-security/csrf/samesite-cookies
35  	 *
36 -	 * @var 'Lax'|'None'|'Strict'
37 +	 * @var Lax|None|Strict
38  	 */
39  	public $samesite = 'Lax';
40  }
41     ----------- end diff -----------

[samsonasik]

That seems related to Format preserving doc, ref :

• Format-preserving AST transformations TODO nikic/PHP-Parser#344
• [FPDP] Format preserving doc printer rectorphp/rector#4334

I may create a workaround patch for this in rector side, eg in the ContentPatcher class when I have a chance:

https://github.com/rectorphp/rector/blob/133322cc658fcd8d5ba162b4a18c7d9d88eba714/src/PhpParser/Printer/ContentPatcher.php#L142-L147

For now, I think we can do with:

/** @var string 'Lax'|'None'|'Strict' */

ref :

• https://getrector.org/demo/5aea2ecd-f2db-4f60-b61d-ccfa06d9c457
• https://phpstan.org/r/03e63c8d-6d30-4bed-bb76-1ca36a0b679c

[paulbalandan]

Try this if you need to be explicit of the type in phpstan.

// for vars
 * @var string
 * @phpstan-var 'Lax'|'None'|'Strict'

// for returns
 * @return object
 * @phpstan-return TObject

Seems phpstan understand those pseudo-docblocks just like in psalm.

---

Deeper fix for the cookie samesite issue is to let phpstan know that we are actually filtering and formatting the allowed values to samesite. This way it can infer that samesite is indeed within the allowed values and the @var string would still be allowed. For the meantime, the fix is unexpected and very clever. 😂

[samsonasik]

@paulbalandan the @phpstan-var 'Lax'|'None'|'Strict' is not work for rector post -processing right now ref https://getrector.org/demo/1d208398-63da-4f18-8e44-00e16b72bb3e

Need a workaround patch in rector side while wait for a real fix in nikic/php-parser side.

[paulbalandan]

It actually works for me though. You need to have both @var and @phpstan-var.

https://getrector.org/demo/7ed7b50f-7193-4f45-bade-322d372d7343
https://phpstan.org/r/5f71cef4-03ec-4adf-8ca4-4d126af63830

[samsonasik]

Ok, that seems a solution, but need different /* */ block to make work in phpstan ref

https://phpstan.org/r/15ddc4ad-57a1-4d3b-ac04-bbb2075dda73

vs

https://phpstan.org/r/421f43cf-2eac-4f5f-98c2-929fbca30543

[paulbalandan]

No need. You just have a typo in your second phpstan ref

[samsonasik]

@paulbalandan oh yeah, you're correct 👍

[MGatner]

Thanks for taking a look guys. Can someone either push the final version you agreed on or suggest it in a review?

[MGatner]

Thank you both. GitHub is not allowing me to commit revisions for some reason, looks like a bug, but I will get these applied later when I can get back to my server with this branch.

[totoprayogo1916]

Is it possible to use this : @var string[Lax|None|Strict] ?

[samsonasik]

@MGatner @paulbalandan the @phpstan-var 'Lax'|'None'|'Strict' seems only working in rector 0.9 (require php 7.3 in 4.1 branch). I think for now, we can use:

/** @var string 'Lax'|'None'|'Strict' */

as I previously suggested for develop branch ( php 7.2 ).

@totoprayogo1916 this patch want a quoted doc.

[samsonasik]

I am going to merge it right now, I will try to fix the error affected in system/Security/Security.php and system/Session/Session.php in separate PR

[samsonasik]

Thank you all, I created new PR for temporary ignore the phpstan notice in Security and Session class, with @todo to clean up in 4.1 for rector 0.9 #4146

[MGatner]

Thanks all. Sorry to be MIA finishing this up - special thanks to @samsonasik