Skip to content

Conversation

@dcolt
Copy link
Collaborator

@dcolt dcolt commented Jan 20, 2026

Currently lacking structure inside the hook scope.

Currently lacking structure inside the hook scope
@dcolt dcolt requested review from bix0r and sunkan January 20, 2026 07:48
@dcolt
Copy link
Collaborator Author

dcolt commented Jan 20, 2026

This would resolve #39

Copy link
Member

@bix0r bix0r left a comment

Choose a reason for hiding this comment

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

I feel like we need more/better test. What about more complex hooks? Just looking at examples from https://www.php.net/manual/en/language.oop5.property-hooks.php, there are quite a few more cases to consider. Of course we might not want to allow all of them, but then we should have tests related to it.


private function removeInvalidErrors(File $phpcsFile, int $stackPtr, TokenCollection $tokens): void
{
$removeError = function (int $line, int $column): void {
Copy link
Member

Choose a reason for hiding this comment

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

Rename $line and $column because you use those variable names in the for loops later. Or just remove them if you don't use them at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing this out.

The loop shouldn't be there at all actually, as I already know where the error is supposed to be.

@dcolt dcolt marked this pull request as draft January 20, 2026 08:13
@sunkan
Copy link
Member

sunkan commented Jan 22, 2026

@dcolt
Can you added the extra test cases biggi mentioned so we can move this forward

@sunkan
Copy link
Member

sunkan commented Jan 23, 2026

@dcolt

Should this be moved from "draft" state?

@dcolt
Copy link
Collaborator Author

dcolt commented Jan 23, 2026

@dcolt

Should this be moved from "draft" state?

No, it's still not complete.
The property hook scopes aren't included in the existing indentation logic, so that's currently what's ongoing with this.

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.

4 participants