Do not mutate clean up functions#1285
Conversation
| private $whitelisted = [ | ||
| 'close', | ||
| 'free', | ||
| 'free_result', |
There was a problem hiding this comment.
I'm not terribly excited about a special list for instance methods. I wish we could have this either configurable, or completed with the means supposed to exists under #1231
There was a problem hiding this comment.
If it was configurable you would need this everywhere ?
How about checking that all of these in the method versions take no argument ? eg $someClass->free() would be ignored but $someClass->free($someVariable) would be allowed to mutate ?
There was a problem hiding this comment.
I think doing so against native calls is ok but extending it to arbitrary classes is not: a user will likely be able to test that its $someClass->free() is called or not
There was a problem hiding this comment.
They return void though ? https://www.php.net/manual/en/mysqli-result.free.php or https://www.php.net/manual/en/event.free could check if void return and 0 arguments and called one of these things ?
There was a problem hiding this comment.
and how do you tell the difference between $mysqliResult->free() and $repository->free()? We do not have type inference at the moment so we cannot distinguish the two and the later may deserve to be covered by infection
There was a problem hiding this comment.
I agree there will be methods missed by doing this, I don't have the stats but most methods I have seen called one of these are methods which have no real observable behaviour.
Could have this as a user config option instead at project level instead of line level ?
There was a problem hiding this comment.
I don't think a user config option for this list is a bad idea.
50811d4 to
e6b1e06
Compare
|
Since we have different opinions on object's methods calls, here is what I suggest:
|
Functions like Close and Free should not be removed by the function call removals. Removing them does not change the tested behaviour, but does free the memory before the script finished / garbage collection is invoked
4795f26 to
3aaf3fe
Compare
Sounds good to me, I have updated the PR to be just the native functions side of things here. |
|
Thank you, @exussum12! |
|
Does it make sense to add |
|
sounds good to me. Feel free to open a PR targeted to 0.17, so we will release it ASAP as 0.17.1 |
Functions like Close and Free should not be removed by the function call
removals. Removing them does not change the tested behavior, but does
free the memory before the script finished / garbage collection is
invoked
This PR: