[HttpFoundation] Add phpstan-pure to UriSigner to avoid unhandled checks #54297
Conversation
|
We usually don't accept this kind of change on LTS branches. Please target 7.1 instead. |
alexander-schranz
requested review from
chalasr,
dunglas,
jderusse and
yceruto
as code owners
alexander-schranz
force-pushed
the
patch-10
branch
from
2f6a25e to
d6d7334
Compare
| @@ -37,6 +37,8 @@ public function __construct(#[\SensitiveParameter] string $secret, string $param | |||
| * | |||
| * The given URI is signed by adding the query string parameter | |||
| * which value depends on the URI and the secret. | |||
| * | |||
| * @phpstan-pure | |||
There was a problem hiding this comment.
As we use psalm in our CI (even though we use it in an incremental mode), we generally use the psalm- prefix rather than the phpstan- one.
this is not true. Most services don't guarantee being pure in Symfony, which allow different implementations (for instance, an implementation storing the result in a Cache would not be pure). Declaring an API as being pure is a very strict requirement for the implementation. |
|
@stof aggree, depends mostly on which service you look at. Cache is sure a sideeffect which means not pure. Most factories, generators, matcher, .. services are pure as they don't have any internal states or write to something / output something. Things like cache, orm, ... are sure no pure methods, also twig Environment::display which does echo is not pure. Sure we can not add blindly pure to everything, mostly I would go the way were we see advantages of having it pure like the UriSigner we can with it avoid bugs. |
|
@alexander-schranz but most of our interface cannot be marked as pure, as this would force all implementations to be pure (which would be a BC break by changing the contract of the interface btw) |
carsonbot
changed the title
|
Not a fan of adding these annotations in core. Can't this be moved to the symfony plugin? |
|
I agree with @nicolas-grekas, also because I don't see why we would add these to this specific class and not elsewhere. |
5.47.1In a project I did refactor some controllers I did stumble over the following code:
Why it may first looks correctly this does nothing and even opens a security issue for the project. Via
phpstan-purestatic code analyzers can find such kind of issue and show a error that this method call was not handled.https://phpstan.org/r/12844f00-e8d8-4f01-8893-8d43ca4efdfe
Adding phpstan-pure was suggested by Ondrej here: phpstan/phpstan-symfony#387.
Technical
phpstan-puremeans that method does not have any sideeffects (editing any internal state), so most service methods could be pure.