Skip to content

[Lock] Make SemaphoreStore::isSupported() internal #24057

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 1, 2017

Conversation

maidmaid
Copy link
Contributor

@maidmaid maidmaid commented Aug 31, 2017

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 /

@@ -33,6 +33,10 @@ class SemaphoreStore implements StoreInterface
*/
public static function isSupported($blocking = null)
{
if (null !== $blocking) {
@trigger_error(sprintf('The "$blocking" argument of the "%s" is deprecated since 3.4 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal call should not trigger deprecation (this argument is used by component console)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how know if call is internal or not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By passing false as second (extra, implicit) argument in each internal call, then triggering only if 1 === func_num_args() || func_get_arg(1). But wait, @jderusse do we really need to expose this argument in 3.4? Would it be enough to not add it and consider it exists only for internal calls?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make it final and move on :)

Copy link
Member

@chalasr chalasr Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas doesn't #24053 (comment) still apply when calling the method (no inheritance)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing will break, so we don't care

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok then :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just mark this method as @internal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@internal used.

@@ -5,3 +5,4 @@ CHANGELOG
-----

* added the component
* deprecated the `$blocking` argument of the `SemaphoreStore::isSupported()` method
Copy link
Member

@jderusse jderusse Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thé component is not released yet. Does it make sense to deprecate things here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's usefull to see deprecations linked to 3.4 here I think, else no textual informations is written elsewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lock was not part of 3.3, so there is nothing to deprecate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, line removed.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 31, 2017
@xabbuh xabbuh removed the Deprecation label Sep 1, 2017
@chalasr chalasr changed the title [Lock] Deprecate argument of SemaphoreStore::isSupported() [Lock] Make SemaphoreStore::isSupported() internal Sep 1, 2017
@chalasr chalasr added the Lock label Sep 1, 2017
@fabpot
Copy link
Member

fabpot commented Sep 1, 2017

Thank you @maidmaid.

@fabpot fabpot merged commit 86f2f25 into symfony:3.4 Sep 1, 2017
fabpot added a commit that referenced this pull request Sep 1, 2017
…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
@maidmaid maidmaid deleted the lock-3.4 branch September 1, 2017 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants