Skip to content

Squiz/SelfMemberReference: update XML doc #1108

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rodrigoprimo
Copy link
Contributor

Description

This PR updates the XML documentation of the Squiz.Classes.SelfMemberReference sniff to address the following:

  • Improve sniff description explicitly mentioning where the spaces are checked and adding context as to when the self keyword is verified.
  • Fix the code examples for checking self case and spaces around the double colon to ensure they trigger the sniff (the sniff checks if the call is made within a class).
  • Improve the titles of the code examples related to spacing around double colons.
  • Remove unnecessary methods from the code examples related to using self instead of the class name when referencing a static member.

Suggested changelog entry

Squiz.Classes.SelfMemberReference: improvements to the XML documentation

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

- Improve sniff description explicitly mentioning where the spaces are
checked and adding context as to when the `self` keyword is verified.
- Fix the code examples for checking `self` case and spaces around the
double colon to ensure they trigger the sniff (the sniff checks if the
call is made within a class).
- Improve the titles of the code examples related to spacing around
double colons.
- Remove unnecessary methods from the code examples related to using
`self` instead of the class name when referencing a static member.
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Hi @rodrigoprimo, thanks for this PR!

I've left two comments inline.
Other than that, I wonder if the first two code comparisons need to have the braces on their own lines - it kind of makes the code samples longer than needed ?

Comment on lines +4 to +6
When referencing static members within a class, the self keyword must be used instead of the
current class name, it must be lowercase, and there must be no spaces before or after the double
colon.
Copy link
Member

Choose a reason for hiding this comment

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

This rephrasing is confusing to me.

"static members within a class" will be interpreted as methods/properties declared with the static keyword, while that's not something the sniff takes into account.

The static in the "local static member reference" in the error messages is about static references, not static members.

Copy link
Contributor Author

@rodrigoprimo rodrigoprimo Jun 2, 2025

Choose a reason for hiding this comment

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

If I understood your point correctly, I failed to consider that even though calling a non-static member with a static reference causes a warning or fatal error depending on the PHP version, it still works in some versions and, more importantly, it is not something that the sniff checks.

What about "When making static references to class members from within the same class" instead of "When referencing static members within a class"?

The description would become:

When making static references to class members from within the same class, the self keyword must be used instead of the current class name, it must be lowercase, and there must be no spaces before or after the double colon.

Or do you prefer the original approach, and then the final description could be:

The self keyword must be used instead of the current class name, it must be lowercase, and there must be no spaces before or after the double colon.

Copy link
Member

Choose a reason for hiding this comment

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

calling a non-static member with a static reference causes a warning or fatal error depending on the PHP version

Actually, as this sniff is about hierarchy keywords, IIRC this won't be a warning nor a fatal error in any PHP version.

Or do you prefer the original approach

I think I do as it makes it more straight-forward to understand what the sniff expects, though the description does contain multiple rules in one description.

Might be worth splitting up the description to have a separate description for each code sample ?

Comment on lines -36 to 62
public static function bar()
{
}

public static function baz()
Copy link
Member

Choose a reason for hiding this comment

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

Why did you choose to remove the bar() methods ? For this specific error, it kind of makes sense to have the function declaration being referenced available in the code sample (though possibly this could be condensed to a one-liner with the braces on the declaration line).

I also wonder if the static in the function signatures should be removed, as it could give the impression - same as the updated description -, that this is about static methods only, while that's not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did you choose to remove the bar() methods ?

I opted to remove the bar() methods to simplify the code example. I also took into consideration that they are not necessary to trigger the sniff (the sniff doesn't check if the method being called exists), that AFAIK similar code examples don't provided the called method like in

and that not including bar() doesn't create a syntax error (despite making the code not functional).

I also wonder if the static in the function signatures should be removed, as it could give the impression - same as the updated description -, that this is about static methods only, while that's not the case.

That is a good point and something that I hadn't considered. I will update the code example to remove static from the function signature.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please update the function signature.

Re: the removed method - I don't see any harm in these being there and it makes it clearer what this is about, so I see no reason why it should be removed.

Note: comparing this to the UselessOverridingMethod sniff is an invalid comparison. This sniff is about references to methods in the same class, while the UselessOverridingMethod sniff is about methods being overloaded from another class, so not the same thing.

@rodrigoprimo
Copy link
Contributor Author

rodrigoprimo commented Jun 2, 2025

Thanks for your review, @jrfnl!

Other than that, I wonder if the first two code comparisons need to have the braces on their own lines - it kind of makes the code samples longer than needed ?

That is a good question. It is not clear to me what coding standards the code examples in the XML documentation should follow. I also prefer to have the opening brace on the same line as the function/class signature. Happy to change it if it is something that is ok to change. I can check, but I have a feeling that most of the XML documentation in this repo has the opening braces on their own lines due to the coding standard used in this repository.

I'm not sure why you are referring just to the two code comparisons and not all of the code comparisons in the documentation for this standard. Is there a reason that I'm missing? If we are going to change the first two code comparisons, I think we should change the third as well.

@jrfnl
Copy link
Member

jrfnl commented Jun 8, 2025

That is a good question. It is not clear to me what coding standards the code examples in the XML documentation should follow.

That's a good point and I don't think we have clear guidelines for this (yet). Maybe it should be based on the standard a sniff is in ? But then, what coding standard should the code samples for the Generic sniffs follow ?

Might be a good idea to open an issue about this for further discussion and decision making.

I'm not sure why you are referring just to the two code comparisons and not all of the code comparisons in the documentation for this standard. Is there a reason that I'm missing?

Yes, the first two examples only have the class ... {} and function {} wrappers to make it valid code, not to demonstrate the issue the sniff is looking for.

For the last example, the class ... {} and function {} wrappers have a functional value to demonstrate the issue.

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.

2 participants