-
-
Notifications
You must be signed in to change notification settings - Fork 8
Create starting samurai autogen npc #1890
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: master
Are you sure you want to change the base?
Conversation
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.
Code Review Summary
Analysis Results:
1 issues found (0 critical, 1 high, 0 medium, 0 low)
Key Findings (Critical & High):
- Potential undefined constant usage in stealth status check in deploy/lib/control/NpcController.php
Files Analyzed
| File Path | Changes | Issues Found |
|---|---|---|
| deploy/lib/control/NpcController.php | 4 | 1 |
This analysis was performed by Evolua. For support, please contact our team.
- Email us @ support@evolua.io
- Run: /evolua-review to start a new code review.
- Visit Evolua for more information
| if ($player->hasStatus(STEALTH) && | ||
| (in_array(strtolower($victim), self::$STEALTH_REMOVING_NPCS) || ($npco && $npco->hasTrait('stealth_removing')))) { |
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.
The code uses STEALTH as a constant in the condition $player->hasStatus(STEALTH), but there is no visible declaration of this constant in the provided code. If this constant is undefined, it will be interpreted as a string literal 'STEALTH', which might cause the hasStatus check to fail silently, allowing players to maintain stealth status incorrectly when attacking NPCs that should remove it.
Impact
This could lead to gameplay imbalance where players can attack certain NPCs while maintaining stealth when they shouldn't be allowed to. This breaks game mechanics and could potentially be exploited by players.
References
Recommendation
Either define the STEALTH constant if it's missing, or use a string literal if that's the intended behavior. If the constant is defined elsewhere in the codebase, consider importing it or using a class constant instead for better maintainability.
if ($player->hasStatus(Player::STEALTH) &&
(in_array(strtolower($victim), self::$STEALTH_REMOVING_NPCS) || ($npco && $npco->hasTrait('stealth_removing')))) {📝 Suggested fix
‼️ IMPORTANT
Please review this suggestion carefully before applying:
- Verify it matches your codebase standards
- Ensure it doesn't introduce new issues
- Test thoroughly after applying
| if ($player->hasStatus(STEALTH) && | |
| (in_array(strtolower($victim), self::$STEALTH_REMOVING_NPCS) || ($npco && $npco->hasTrait('stealth_removing')))) { | |
| if ($player->hasStatus(Player::STEALTH) && | |
| (in_array(strtolower($victim), self::$STEALTH_REMOVING_NPCS) || ($npco && $npco->hasTrait('stealth_removing')))) { |
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.
Hmmm, I think this is the old “defined globally” constant problem.
NinjaWars Functional Testing
|
||||||||||||||||||||||||||||||
| Project |
NinjaWars Functional Testing
|
| Branch Review |
feat/npm-updates
|
| Run status |
|
| Run duration | 02m 00s |
| Commit |
|
| Committer | Coco R |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
14
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Purpose of PR:
Before
After
For Non-Hotfixes:
Attached Screenshot of my change:
Things that make review take longer:
(remove lines that do not apply to this PR)
Things that make review faster and easier:
(check box with an x where it applies)
Preview results in my branch at the url: