Conversation
📝 WalkthroughWalkthroughAdded a release note. Refactored CircleParser to delegate circle construction and enforce radius defaulting to 1 for missing/invalid/non-positive inputs. Updated LegacyModel\Circle with typed properties, moved radius validation into setter, and adjusted method return types. Tests were extended to cover new radius behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/LegacyModel/Circle.php(2 hunks)
🔇 Additional comments (2)
src/LegacyModel/Circle.php (2)
19-20: Typed properties for centre and radius: LGTMGood move to strict typing here. This tightens invariants and avoids accidental null/uninitialized state. Constructor routing through setters ensures validation runs consistently.
44-46: No internal string/null usages detected for Circle settersA repo-wide search revealed only one instantiation of Circle—and it passes the correct types:
- src/WikitextParsers/CircleParser.php:77
return new Circle(
$this->stringToLatLongValue($circleData[0]), ← LatLongValue
$this->extractRadius($circleData) ← float (cast in extractRadius)
);No calls to setCircleRadius() or setCircleCentre() pass a raw string or null. Thus, there’s no internal BC break.
Recommendation: Communicate this new strict typing to any external consumers (e.g., in release notes or an upgrade guide) so they can adjust numeric-string or null arguments into floats and LatLongValue objects.
| public function setCircleRadius( float $circleRadius ): void { | ||
| if ( $circleRadius <= 0 ) { | ||
| throw new InvalidArgumentException( '$circleRadius must be greater than zero, got "' . $circleRadius . '"' ); | ||
| } | ||
|
|
There was a problem hiding this comment.
NaN and Infinity bypass validation — treat non-finite values as invalid
Comparison with NaN returns false, so NaN currently slips through and gets assigned. Infinity should also be rejected. Tighten the guard to only allow finite, positive values.
Apply this diff:
- if ( $circleRadius <= 0 ) {
- throw new InvalidArgumentException( '$circleRadius must be greater than zero, got "' . $circleRadius . '"' );
- }
+ if ( $circleRadius <= 0 || is_nan( $circleRadius ) || is_infinite( $circleRadius ) ) {
+ throw new InvalidArgumentException(
+ sprintf( '$circleRadius must be a finite number greater than zero, got %s', var_export( $circleRadius, true ) )
+ );
+ }Optionally, add a small unit test to assert the setter rejects NaN/INF (and that the parser never produces them).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function setCircleRadius( float $circleRadius ): void { | |
| if ( $circleRadius <= 0 ) { | |
| throw new InvalidArgumentException( '$circleRadius must be greater than zero, got "' . $circleRadius . '"' ); | |
| } | |
| public function setCircleRadius( float $circleRadius ): void { | |
| - if ( $circleRadius <= 0 ) { | |
| - throw new InvalidArgumentException( '$circleRadius must be greater than zero, got "' . $circleRadius . '"' ); | |
| - } | |
| + if ( | |
| + $circleRadius <= 0 | |
| + || is_nan( $circleRadius ) | |
| + || is_infinite( $circleRadius ) | |
| + ) { | |
| + throw new InvalidArgumentException( | |
| + sprintf( | |
| + '$circleRadius must be a finite number greater than zero, got %s', | |
| + var_export( $circleRadius, true ) | |
| + ) | |
| + ); | |
| + } |
🤖 Prompt for AI Agents
In src/LegacyModel/Circle.php around lines 52 to 56, the setter currently only
checks $circleRadius <= 0 which allows NaN and Infinity to pass; update the
guard to reject non-finite values too by verifying the value is finite and
greater than zero (e.g. use PHP's is_finite($circleRadius) and $circleRadius >
0), and throw the same InvalidArgumentException when the check fails; optionally
add a unit test asserting the setter rejects NAN, INF and -INF (and that any
parser code does not produce them).
2 participants
Summary by CodeRabbit
Bug Fixes
Documentation
Tests