fix: Squiz.Arrays.ArrayDeclaration for static closures

[michalbundyra]

Description

@jrfnl it turns out to be a quick fix

Suggested changelog entry

Related issues/external references

Fixes #368

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.

[jrfnl]

@michalbundyra Thanks for working on this.

When looking at the patch, I have three questions:

1. Why did you add a separate unit test case file instead of adding the code sample to the 1 (long arrays) + 2 (short arrays) test case files ?
2. In the comment, you mention that the value pointer in $indices is not set correctly for this type of code. The current fix is basically a work-around for that. Is there a reason to use a work-around instead of fixing it when $indices is set originally ?
3. When I run the following command on the test case file, I get a different result from what is in the committed .fixed file, even though the tests pass. Any idea why this is happening ?

   phpcbf -ps ./squiz/Tests/Arrays/ArrayDeclarationUnitTest.3.inc --standard=Squiz --sniffs=Squiz.Arrays.ArrayDeclaration --suffix=.fixed

[michalbundyra]

@jrfnl thanks for reviewing.

1. I wasn't aware about this. Just changed and added this case to existing files and drop the other file.
2. Yeah, it was a workaround... i did it as there was the if statement already, but just checked and proper solution seems to be also quite simply.
3. Honestly I have no idea... maybe now it's sorted? 🤔

[jrfnl]

Hi @michalbundyra, thanks for making those updates. Looks good to me. Merging!

Re: 3 - this is still happening, but not just for your code sample in the 1/2 test case files. Considering the status of the sniff (on the shortlist for removal), I'm going to let this go as it's not worth the time to investigate this further.

[jrfnl]

@michalbundyra I think I've found a bug in this patch - consider an array entry doing static::methodCall(). This will now result in an Undefined array key "scope_closer".

Would you like to submit a patch for this ? Or would you like me to follow up on this ?

And as a follow-up is now needed, we should probably make sure the tests contain all possible uses of static within an array, including instanceof static and static as a return type for an arrow function/closure (and whatever other uses we can think of).

[michalbundyra]

@jrfnl I had a bad feeling that it might end up like that, but I haven't found yet the failing example in my head.

You are right about these... so maybe my first approach "workaround" was better here, then?

If so, I can create a new PR with this change and adding other test cases.

[jrfnl]

@michalbundyra I think the change is in the right place in the flow of the logic, but I think the check for T_STATIC needs to be split off from the check for the function declaration tokens and done before those (probably with an interim variable).

So something along the lines of:

• Set temporary variable to $nextToken .
• Check for a T_STATIC token, if so, update the temporary variable.
• Check for function declaration tokens using the temporary variable, if so, do the other variable updates.

[michalbundyra]

@jrfnl please see #379 - if you'd like prefer to take another approach feel free to update :) I think it this might be not the best one, but I think it's good enough not to break anything and ... especially if the sniff is going to be deprecated, I wouldn't care too much here :)

[jrfnl]

Thanks @michalbundyra, I'll have a look tomorrow.