PHP iterator signature update #4
Conversation
…terator interface
|
Warning Rate limit exceeded@kumy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdded strict typing across src/Query.php and src/Session.php: properties, constructors, method parameters, and return types were annotated and internal logic adjusted to match typed contracts (iterator behavior, buffering, send/receive/error handling). Changes
Sequence Diagram(s)sequenceDiagram
participant Q as Query
participant S as Session
Note over Q,S `#E8F5E9`: Typed interactions and error checks
Q->>S: execute(query : string)
S-->>Q: resultId : string
Q->>S: exec(cmd : string, arg : string)
alt ok
S-->>Q: response string
else error
S-->>Q: throws BaseXException (info ?? 'Unknown error')
end
Q->>Q: more() -> fetch & buffer into cache (pos:int)
Q->>Q: next() advances pos if more() true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Session.php (1)
158-165: Add error handling forsocket_recv()inread()methodThe
read()method (line 161) does not check ifsocket_recv()fails. Per PHP documentation,socket_recv()returnsfalseon error, but the code assigns this directly to$this->bsize(declared asint). When the next call toread()occurs, the condition$this->bpos == $this->bsizemay still evaluate, and the method would return stale buffer data instead of detecting the socket failure.This matches the error-handling pattern already established elsewhere in the file (e.g.,
socket_connect()check at line 26). Add the following:private function read(): string { if ($this->bpos == $this->bsize) { - $this->bsize = socket_recv($this->socket, $this->buffer, 4096, 0); + $this->bsize = socket_recv($this->socket, $this->buffer, 4096, 0); + if ($this->bsize === false) { + throw new BaseXException("Socket read error: " . socket_last_error($this->socket)); + } $this->bpos = 0; } return $this->buffer[$this->bpos++]; }
🧹 Nitpick comments (2)
src/Session.php (1)
16-20: Property type declarations look good.The type declarations are appropriate. The
mixedtype for$socketcorrectly handles the fact thatsocket_create()can returnSocket|false. All properties are properly initialized with type-safe defaults.Consider using
Socket|falsefor PHP 8.1+ for more precise typing:- protected mixed $socket; + protected Socket|false $socket;This would provide better IDE support and type safety, though
mixedis perfectly acceptable for broader PHP 8.0 compatibility.src/Query.php (1)
1-11: Consider adding strict type enforcement.While the type hints are correctly added, consider adding
declare(strict_types=1);after the opening PHP tag to enable strict type checking at runtime. This would catch type-related bugs earlier and is a best practice for newly typed code.Apply this pattern to both files:
<?php +declare(strict_types=1); + /* * PHP client for BaseX.Note: This could be a breaking change if any client code relies on PHP's type coercion, so verify compatibility before applying.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Query.php(3 hunks)src/Session.php(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Session.php (2)
src/Query.php (5)
info(73-76)__construct(26-30)execute(42-45)Query(13-116)close(83-86)src/BaseXException.php (1)
BaseXException(13-14)
src/Query.php (1)
src/Session.php (6)
Session(13-201)__construct(22-49)query(77-80)execute(57-69)info(125-128)close(133-137)
🔇 Additional comments (11)
src/Session.php (6)
22-22: LGTM! Constructor parameter types are correct.The explicit parameter types match the method's usage and improve type safety.
57-68: Excellent improvements to type safety and error handling.The return type declaration correctly reflects the method's behavior, and the null coalescing operator on line 66 ensures a meaningful error message even when
$this->infois null.
77-118: LGTM! Method signatures are well-typed.All return type declarations are accurate and improve type safety. The
voidreturn types for command methods (create,add,replace,store) correctly indicate these methods are called for their side effects.
125-137: LGTM! Return types are correct.The nullable return type for
info()correctly reflects that the info property may be null, andvoidforclose()is appropriate.
149-156: LGTM! Return type is correct.The
stringreturn type accurately reflects this method's behavior.
167-200: LGTM! Method signatures are well-typed.All type declarations are correct. The null coalescing operator on line 172 provides consistent error handling with the
execute()method.src/Query.php (5)
15-18: LGTM! Property types are correct and well-initialized.The type declarations accurately reflect the property usage and align with the typed
SessionAPI.
26-30: LGTM! Constructor parameter types are correct.The explicit types enforce the dependency on
Sessionand improve type safety throughout the class.
32-45: LGTM! Method signatures are well-typed.The parameter and return types accurately reflect each method's behavior.
47-96: LGTM! All method signatures are correctly typed.The type declarations improve API clarity and type safety. The
exec()method'sstringreturn type ensures consistent behavior across all command execution methods.
98-115: Excellent! Iterator interface compliance achieved.These return type declarations directly address the PR's main objective by ensuring compatibility with PHP 8+'s
Iteratorinterface requirements. Themixedreturn types forcurrent()andkey()match the interface specification, eliminating the deprecation warnings mentioned in the PR description.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Session.php (1)
22-28: Critical: Check for socket_create failure before using the socket.If
socket_create()returnsfalse, line 26 will attempt to passfalsetosocket_connect(), which expects aSocketobject in PHP 8+. This will cause a type error.Apply this diff to check for socket creation failure:
public function __construct(string $hostname, int $port, string $user, string $password) { // create server connection $this->socket = socket_create(AF_INET, SOCK_STREAM, SOL_TCP); + if ($this->socket === false) { + throw new BaseXException("Failed to create socket: " . socket_strerror(socket_last_error())); + } if (!socket_connect($this->socket, $hostname, $port)) { throw new BaseXException("Can't communicate with server."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Query.php(3 hunks)src/Session.php(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Query.php (1)
src/Session.php (6)
Session(13-204)__construct(22-49)query(77-80)execute(57-69)info(125-128)close(133-137)
src/Session.php (2)
src/Query.php (5)
info(73-76)__construct(26-30)execute(42-45)Query(13-116)close(83-86)src/BaseXException.php (1)
BaseXException(13-14)
🔇 Additional comments (11)
src/Query.php (6)
15-18: Property type declarations look good.The property types are appropriate and align with PHP 8+ requirements for strict typing.
26-30: Constructor signature is correct.The typed parameters properly match the property declarations.
66-71: Non-standard Iterator pattern - verify intentional design.The
next()method conditionally increments$posonly whenmore()returns true. The standard Iterator contract expectsnext()to advance unconditionally, withvalid()checking if the new position is valid. This forward-only approach may be intentional, but please confirm this behavior is correct for BaseX query result iteration.
88-96: Proper error handling and type safety.The typed signature and exception handling align well with PHP 8+ best practices.
98-115: Iterator interface implementation is PHP 8+ compatible.All method signatures correctly match the
Iteratorinterface requirements, which resolves the deprecation notices. The emptyrewind()implementation is acceptable for a forward-only iterator.
47-64: Verified: Doubleok()call pattern correctly implements BaseX protocol.The code properly handles the BaseX wire protocol. The
ok()method (Session.php:190-192) reads one byte from the socket, and its@internaldocumentation confirms it is "not idempotent." The pattern inmore()is correct: the firstok()call in the while loop checks for the end-of-results marker (null byte), and the secondok()call reads the status byte. Each invocation consumes one byte as intended.src/Session.php (5)
17-20: Property types are correctly declared.These property type declarations are appropriate and properly initialized.
57-69: Good error handling with defensive null check.The typed signature and use of the null coalescing operator (line 66) provide good type safety and prevent potential null reference issues.
77-137: Public method signatures are properly typed.All method signatures correctly use parameter and return types, which provides better type safety and IDE support.
158-168: Excellent defensive error handling.The added check for
socket_recvfailure (lines 162-164) with detailed error information is a solid improvement for debugging socket communication issues.
170-203: Internal methods are properly typed.All private and public method signatures include appropriate type hints and return types, completing the PHP 8+ compatibility improvements.
Hi @jschroed91
would you please consider this PR to bring fixes for PHP 8+ to avoid deprecation notices like:
72785b6 is the minimal required fix, and as I was on it I also added type hints for other methods in b4b2b8f.
Thanks
Edit: Also added the fix proposed by the review
Summary by CodeRabbit