Skip to content
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

CS: use self where appropriate #409

Merged
merged 1 commit into from
Oct 23, 2020
Merged

CS: use self where appropriate #409

merged 1 commit into from
Oct 23, 2020

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 22, 2020

Use self when referencing a (static) class member from within the same class.

Use self when referencing a (static) class member from within the same class.
@jrfnl jrfnl added this to the 1.7.1 milestone Oct 22, 2020
@ozh
Copy link
Collaborator

ozh commented Oct 23, 2020

Is this good practice ? I mean, if at some point things are refactored and bits are moved to a subclass, it's easy to forget this.

What's the benefit ?

@jrfnl
Copy link
Member Author

jrfnl commented Oct 23, 2020

@ozh There are up and down sides to this.
By the same logic you use above, the class could be renamed and this would be one use of the name which now won't need to be changed anymore because of using self.

Generally speaking, it increases the code readability as a dev now knows instantly that the method/property is intended to be one of this class.

@schlessera
Copy link
Member

Using self/static is considered good practice, indeed.

I'm going to merge this as is, but in case we raise the PHP minimum version to 5.3 or higher, we absolutely need to replace self with static (late static binding was only introduced with PHP 5.3), as this constitutes a bug. The problem here is that the class is not final, and right now if you'd extend the class to override get_certificate_path(), it would just break. a non-final class should in general just use self (or its class name, which is the same thing) when referring to a private property/method.

@schlessera schlessera merged commit dc7fa1a into master Oct 23, 2020
@schlessera schlessera deleted the feature/cs-use-self branch October 23, 2020 14:37
@jrfnl
Copy link
Member Author

jrfnl commented Oct 23, 2020

Agreed. I actually considered using static (forgetting that the repo has a 5.2 minimum) and for now decided against it as it would be a BC-break, considering that it originally used the hard-coded class name.

When older PHP support is dropped in the next major, such a BC-break would not be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants