[Lock] Remove old version check #24053
Conversation
| @@ -37,7 +37,7 @@ public static function isSupported($blocking = null) | |||
| return false; | |||
| } | |||
|
|
|||
| if ($blocking === false && \PHP_VERSION_ID < 50601) { | |||
There was a problem hiding this comment.
the removed check is always "false", to the "&&" is false also, so the full "if" block can be removed
There was a problem hiding this comment.
Right. Block removed.
There was a problem hiding this comment.
in master, the removed check is always true
|
we have several similar thing in tests, can you remove them too? |
| @@ -37,10 +37,6 @@ public static function isSupported($blocking = null) | |||
| return false; | |||
There was a problem hiding this comment.
return extension_loaded('sysvsem');
There was a problem hiding this comment.
Can I remove $blocking arg? Or is it a BC?
There was a problem hiding this comment.
3.4 is not released yet. IMHO, you can remove it too (this method is used in the Symfony\Component\Console\Command\LockableTrait). ping @nicolas-grekas
There was a problem hiding this comment.
removing an argument is BC I think
(but 3.4 will be release with the arg isn't it? so the fact it's not released yet shouldn't matter)
There was a problem hiding this comment.
if one starts overriding/calling this method in 3.4 and we remove this argument in 4.0 without adverting in 3.4, one will keep passing/defining this argument when calling/overriding the method. Yet that would be just dead code, but if we need to add a new argument to this method in the future for whatever reason, the new argument will receive the value intended for the old arg, leading to WTF moments. Seems fragile to me
There was a problem hiding this comment.
(we don't make free BC breaks in major releases)
There was a problem hiding this comment.
indeed, this argument will stay in 3.4 (because 3.x is compatible with php 5.5 and need this check) and will be released.
There was a problem hiding this comment.
Ok. What about add a deprecated notice in 3.4 and remove this argument in 4.0?
There was a problem hiding this comment.
@maidmaid sounds good enough to me, as usual the deprecation should not be triggered for internal calls.
Tests seem ok. What exactly do you think? |
0321809 to
ec91588
Compare
|
Changes done. |
|
Sounds good to me. |
|
@maidmaid I meant the test case :) |
|
@xabbuh Sorry, I don't understand well, could you be more precise please? |
|
@maidmaid Take a look at the line in the test I linked to in #24053 (comment). There we still pass |
|
The test case should be updated, it has false as an argument |
|
Oh, of course, I confused the trait with the test, sorry! Changes done. |
|
Can you also remove the |
|
Done. |
| 4.0.0 | ||
| ----- | ||
|
|
||
| * removed the `$blocking` argument of the `SemaphoreStore::isSupported()` method |
There was a problem hiding this comment.
no need to document this as the method is internal
| } else { | ||
| $store = new FlockStore(sys_get_temp_dir()); | ||
| } | ||
| $store = SemaphoreStore::isSupported() ? new SemaphoreStore() : new FlockStore(sys_get_temp_dir()); |
There was a problem hiding this comment.
I would just drop the false argument to minimise the diff
| } else { | ||
| $store = new FlockStore(sys_get_temp_dir()); | ||
| } | ||
| $store = SemaphoreStore::isSupported() ? new SemaphoreStore() : new FlockStore(sys_get_temp_dir()); |
There was a problem hiding this comment.
I would just drop the false argument to minimise the diff
|
Changes done. |
…maid) This PR was merged into the 3.4 branch. Discussion ---------- [Lock] Make SemaphoreStore::isSupported() internal | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24053 (comment) | License | MIT | Doc PR | / Commits ------- 86f2f25 Mark SemaphoreStore::isSupported() as internal
|
Thank you @maidmaid. |
This PR was merged into the 4.0-dev branch. Discussion ---------- [Lock] Remove old version check | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23724 | License | MIT | Doc PR | / Because minimal required version PHP is currenty 7.1.3. ping @jderusse Commits ------- d817f98 Remove old version check
Because minimal required version PHP is currenty 7.1.3.
ping @jderusse