-
Notifications
You must be signed in to change notification settings - Fork 57
Description
Hey team!
I've been looking at the Request class architecture and noticed an interesting design challenge that I think is worth discussing.
I was examining how the Mcp\Schema\JsonRpc\Request
class handles the $id
property and noticed that it's currently only populated through the fromArray
method, but not when objects are created directly via constructors (like in Mcp\Schema\Request\CallToolRequest
). This creates a situation where users can instantiate request objects through constructors, but calling getId()
on those instances will fail since the $id
property remains uninitialized.
This pattern might lead us into some runtime issues when developers naturally try to create requests (for example, for tests) using the constructor and then attempt to access the ID. The current API suggests that both creation paths are valid, but only one actually works completely.
I'm thinking we might want to consider one of two approaches to make this more robust:
Option 1: Add ID parameter to constructors
public function __construct(
string|int $id, // Add this parameter
public readonly string $name,
public readonly array $arguments,
) {
$this->id = $id; // Initialize it here
}
Option 2: Make constructors private (I prefer this option)
private function __construct(
public readonly string $name,
public readonly array $arguments,
) {
}
abstract class Request {
// Keep fromArray as the single entry point
public static function fromArray(array $data): self { ... }
}
What do you think about these approaches? Option 1 maintains the current flexibility while ensuring consistency, while Option 2 creates a single, well-defined creation path that guarantees proper initialization.