Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Lock] Remove old version check#24053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returnfalse; | ||
| } | ||
| if ($blocking ===false && \PHP_VERSION_ID <50601) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right. Block removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in master, the removed check is always true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nvm
jderusse commentedAug 31, 2017
we have several similar thing in tests, can you remove them too? |
| publicstaticfunctionisSupported($blocking =null) | ||
| { | ||
| if (!extension_loaded('sysvsem')) { | ||
| returnfalse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
return extension_loaded('sysvsem');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can I remove$blocking arg? Or is it a BC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
3.4 is not released yet. IMHO, you can remove it too (this method is used in theSymfony\Component\Console\Command\LockableTrait). ping@nicolas-grekas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
removing an argument is BC I think
(but 3.4 will be releasewith the arg isn't it? so the fact it's not released yet shouldn't matter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(we don't make free BC breaks in major releases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok. What about add a deprecated notice in 3.4 and remove this argument in 4.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@maidmaid sounds good enough to me, as usual the deprecation should not be triggered for internal calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see#24057
maidmaid commentedAug 31, 2017
Tests seem ok. What exactly do you think? |
jderusse commentedAug 31, 2017
0321809 toec91588Comparemaidmaid commentedAug 31, 2017
Changes done. |
jderusse commentedAug 31, 2017
Sounds good to me. |
xabbuh commentedSep 1, 2017
maidmaid commentedSep 1, 2017
xabbuh commentedSep 1, 2017
@maidmaid I meant the test case :) |
maidmaid commentedSep 1, 2017
@xabbuh Sorry, I don't understand well, could you be more precise please? |
xabbuh commentedSep 1, 2017
@maidmaid Take a look at the line in the test I linked to in#24053 (comment). There we still pass |
Simperfit commentedSep 1, 2017
The test case should be updated, it has false as an argument |
maidmaid commentedSep 1, 2017
Oh, of course, I confused the trait with the test, sorry! Changes done. |
xabbuh commentedSep 1, 2017
Can you also remove the |
maidmaid commentedSep 1, 2017
Done. |
| 4.0.0 | ||
| ----- | ||
| * removed the`$blocking` argument of the`SemaphoreStore::isSupported()` method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no need to document this as the method is internal
| }else { | ||
| $store =newFlockStore(sys_get_temp_dir()); | ||
| } | ||
| $store = SemaphoreStore::isSupported() ?newSemaphoreStore() :newFlockStore(sys_get_temp_dir()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would just drop thefalse argument to minimise the diff
| }else { | ||
| $store =newFlockStore(sys_get_temp_dir()); | ||
| } | ||
| $store = SemaphoreStore::isSupported() ?newSemaphoreStore() :newFlockStore(sys_get_temp_dir()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would just drop thefalse argument to minimise the diff
maidmaid commentedSep 1, 2017
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
fabpot commentedSep 1, 2017
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@jderusseCommits-------d817f98 Remove old version check
Because minimal required version PHP is currenty 7.1.3.
ping@jderusse