Verify json serializability of collector data in single process run #2881
Conversation
staabm
force-pushed
the
collector-serializing
branch
from
5ded329 to
64e17de
Compare
staabm
force-pushed
the
collector-serializing
branch
from
64e17de to
00a20d4
Compare
|
This pull request has been marked as ready for review. |
staabm
force-pushed
the
collector-serializing
branch
from
df9698e to
dc140e0
Compare
|
|
||
| $errors[] = $this->buildCollectedDataError( | ||
| sprintf( | ||
| 'Data collected by %s is not JSON decodable and will not work in parallel analysis.', |
There was a problem hiding this comment.
I think we need to be more precise here. I could not come up with a better error message.
the most important point is, that a datastructure needs to fulfill json_decode(json_encode($data)) === $data
| } | ||
|
|
||
| $restored = json_decode($json); | ||
| if ($restored === $collected->getData()) { |
There was a problem hiding this comment.
thinking about this, is this actually the correct check?
if there is an object that implements JsonSerializable returned from a collector it will encode fine, but when decoded it will be an stdClass meaning the === check would fail
if it's intended that you can use JsonSerializable in collectors then the check needs to be more robust
There was a problem hiding this comment.
you describe exactly the case, why this rule was created. the behaviour of phpstan is different depending on whether phpstan is running with parallel worker or with a single worker (e.g. in --debug), since the collected data is only json encoded in the the worker and decoded in the main thread in parallel, which means when you put a JsonSerializable object into collected data it will work differently depending on the worker scenario.
in single worker mode the collector data is passed thru the php process 1:1 and does not get influenced by json encode/decode
There was a problem hiding this comment.
ah my misunderstanding, when Ondrej said
This type would not be precise. Anything that is JSON-serializable works.
i thought the intention was that JsonSerializable should be permitted, thanks for clearing that up
@ondrejmirtes would the new rule check the generic template type of ——-
we need two way json encode/decodability. Encode-ability alone - as we have it before this PR - only works in single worker mode, which actually means JsonSerializable objects are hard to work with in real world use-cases. see #2881 (comment) |
refs phpstan/phpstan#10453 (reply in thread)