-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: Code cleanup Pt1 #37
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
base: refactor/code-cleanup
Are you sure you want to change the base?
Conversation
Ninerian
commented
Oct 29, 2025
- added code quality dependencies
- raised phpstan level to 5 and fixed all errors
- updated phpunit to 10
e88e6fe to
3300537
Compare
- Updated phpstan.neon.dist to level 4 - Removed unused expressions in PluginSessionTest - Fixed always-true ternary in SSODataTest - Refined assertion in SSOTokenTest to avoid redundant type check - Removed unreachable code after markTestSkipped statements - Added proper assertions to make mock object usage meaningful 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Updated phpstan.neon.dist to level 5 - Fixed type parameter in HasInstanceId::hasInstanceId() method from Plain to UnencryptedToken - Removed unused Plain token import - Aligns with lcobucci/jwt library token type hierarchy 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
acc47ef to
0ed5e27
Compare
| branches: | ||
| - 'main' | ||
| - '**/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the goal here? Why not just
| branches: | |
| - 'main' | |
| - '**/**' | |
| branches: | |
| - '**' |
| branches: | ||
| - 'main' | ||
| - '**/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
| * @see \Staffbase\plugins\sdk\SSOToken::CLAIM_AUDIENCE | ||
| */ | ||
| const CLAIM_AUDIENCE = 'aud'; | ||
| public const CLAIM_AUDIENCE = 'aud'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to switch to public when it is already marked as deprecated? 🤔
| $this->assertEquals($sessionId, session_id()); | ||
| } | ||
|
|
||
| public function testDestroyOtherSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the plan for this empty test?
| $session->destroySession($sessionHash); | ||
| } | ||
|
|
||
| public function testDestroyOwnSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the plan for this empty test?
| # CodeSniffer | ||
| phpcs.xml | ||
|
|
||
| /vendor/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is already covered by line 13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to have this information rather in .github/copilot-instructions.md
It would be cool to have some general file where all LLMs are looking into.
Since Claude itself is not (yet) company wide approved. It might be helpful for you to keep the CLAUDE.md on your machine and link the .github/copilot-instructions.md file in it where all the information is moved to.