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.

Sign up for GitHub

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 5 commits into from
Jun 17, 2025
Merged

Squiz/SelfMemberReference: update XML doc #1108

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d188297
Squiz/SelfMemberReference: update XML doc
rodrigoprimo May 28, 2025
3775cfc
Create a separate `<standard>` block for each `<code_comparison>` block
rodrigoprimo Jun 13, 2025
dba7017
Add back the bar() method in the third example
rodrigoprimo Jun 13, 2025
a008fdd
Remove the static keyword from the third example as it is not necessa…
rodrigoprimo Jun 13, 2025
ce50679
Make the code examples shorter by not having the opening braces on a …
rodrigoprimo Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions src/Standards/Squiz/Docs/Classes/SelfMemberReferenceStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,56 @@
<documentation title="Self Member Reference">
<standard>
<![CDATA[
The self keyword should be used instead of the current class name, should be lowercase, and should not have spaces before or after it.
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 •
edited
Loading

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Perhaps I'm still missing something, but here is a code example that illustrates what I have in mind. This example is a fatal error in PHP >= 8.0, but it works with a notice in older versions. It is an example of calling a non-static member with a static reference, and it does trigger the sniff.

https://3v4l.org/gRAUG

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

I went ahead and created a <standard> block for each of the <code_comparison> blocks.

Copy link
Member

Choose a reason for hiding this comment

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

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

Perhaps I'm still missing something, but here is a code example that illustrates what I have in mind. This example is a fatal error in PHP >= 8.0, but it works with a notice in older versions. It is an example of calling a non-static member with a static reference, and it does trigger the sniff.

https://3v4l.org/gRAUG

I see what you mean and now understand the confusion.

I was talking about this: https://3v4l.org/160e6

I went ahead and created a <standard> block for each of the <code_comparison> blocks.

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean and now understand the confusion.
I was talking about this: https://3v4l.org/160e6

Ah, I see, thanks for the code sample.

]]>
</standard>
<code_comparison>
<code title="Valid: Lowercase self used.">
<![CDATA[
<em>self</em>::foo();
class Bar
{
public function baz()
{
<em>self</em>::foo();
}
}
]]>
</code>
<code title="Invalid: Uppercase self used.">
<![CDATA[
<em>SELF</em>::foo();
class Bar
{
public function baz()
{
<em>SELF</em>::foo();
}
}
]]>
</code>
</code_comparison>
<code_comparison>
<code title="Valid: Correct spacing used.">
<code title="Valid: No spaces around the double colon.">
<![CDATA[
self<em></em>::<em></em>foo();
class Bar
{
public function baz()
{
self<em></em>::<em></em>foo();
}
}
]]>
</code>
<code title="Invalid: Incorrect spacing used.">
<code title="Invalid: Spaces around double colon operator.">
<![CDATA[
self<em> </em>::<em> </em>foo();
class Bar
{
public function baz()
{
self<em> </em>::<em> </em>foo();
}
}
]]>
</code>
</code_comparison>
Expand All @@ -33,10 +59,6 @@ self<em> </em>::<em> </em>foo();
<![CDATA[
class Foo
{
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

PHP_CodeSniffer/src/Standards/Generic/Docs/CodeAnalysis/UselessOverridingMethodStandard.xml

Line 26 in 52dd82b

<em>parent::bar();</em>
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
<em>self</em>::bar();
Expand All @@ -48,10 +70,6 @@ class Foo
<![CDATA[
class <em>Foo</em>
{
public static function bar()
{
}

public static function baz()
{
<em>Foo</em>::bar();
Expand Down