Fix return value for update

[abnegate]

Summary by CodeRabbit

• Breaking Changes

  • Certain client write operations now return integers (e.g., affected count) instead of a chainable instance.
  • The exception factory now returns the base exception type, which may affect subclass overrides.

• Improvements

  • Read/Write concern helpers accept broader input types (strings/arrays, plus integers for write concerns) and return normalized arrays for consistency.

• Chores

  • Relaxed static analysis settings in the check script to improve developer workflow.

[coderabbitai]

Walkthrough

Updates adjust Composer’s phpstan script, change Client methods’ return types and parameter unions, and modify Exception::fromResponse to return self instead of static, including corresponding PHPDoc updates.

Changes

Cohort / File(s)	Summary
Composer scripts composer.json	Script "check" now runs phpstan without --level=max: vendor/bin/phpstan analyse --memory-limit=1G src.
Client API return types and signatures src/Client.php	update(...) and upsert(...) now return int (was self). createWriteConcern(...) accepts array|string|int and returns array. createReadConcern(...) accepts array|string and returns array. Docblocks updated accordingly.
Exception factory return type src/Exception.php	fromResponse(...) return type changed from static to self; instantiation uses new self(...). PHPDoc updated from @return static to @return self.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Client
  participant DB as Database
  participant Ex as Exception

  Caller->>Client: update()/upsert()(args)
  Client->>DB: execute write
  alt success
    DB-->>Client: affectedCount (int)
    Client-->>Caller: int
  else error
    Client->>Ex: Exception::fromResponse(error, opType)
    Ex-->>Client: Exception (self)
    Client-->>Caller: throw Exception
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Upsert #19 — Adjusts Client upsert/update method signatures/behavior, overlapping with the same methods modified here.

Poem

A nibble of types, a hop through the night,
Returns turn to numbers—so tidy, so right.
Exceptions now self, not static in flight,
Composer hums stan with a gentler light.
Thump goes my paw: the build feels tight. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title highlights the change to the update method’s return value, which is a valid part of this changeset, making it relevant even though it does not cover other return type and configuration updates included in the pull request so it still passes the partial relevance criteria.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-update-return

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/Client.php (1)

809-846: Cast update() result to int
parseResponse() can return a stdClass when the response lacks an n property (e.g. only nModified), violating : int. On line 846, replace the direct return with something like:

$result = $this->query($command);
return is_int($result)
    ? $result
    : (int) ($result->n ?? $result->nModified ?? 0);

🧹 Nitpick comments (3)

src/Client.php (3)

658-660: Empty string check for _id may be overly restrictive.

The condition !isset($docObj->_id) || $docObj->_id === '' rejects documents where _id is explicitly set to an empty string. While empty-string IDs are unusual, MongoDB technically allows them. If the intent is to generate UUIDs only when _id is absent or null, consider using empty($docObj->_id) or explicitly checking for null.

Apply this diff if you want to allow empty strings as valid IDs:

-if (!isset($docObj->_id) || $docObj->_id === '') {
+if (!isset($docObj->_id) || $docObj->_id === null) {
     $docObj->_id = $this->createUuid();
 }

---

732-734: Empty string check for _id may be overly restrictive.

Same concern as in the insert() method: the condition !isset($docObj->_id) || $docObj->_id === '' rejects documents where _id is explicitly set to an empty string. MongoDB technically allows empty-string IDs. If the intent is to generate UUIDs only when _id is absent or null, consider using empty($docObj->_id) or explicitly checking for null.

Apply this diff if you want to allow empty strings as valid IDs:

-if (!isset($docObj->_id) || $docObj->_id === '') {
+if (!isset($docObj->_id) || $docObj->_id === null) {
     $docObj->_id = $this->createUuid();
 }

---

1542-1542: Potential exception when ending sessions during cleanup.

At line 1542, endSessions($activeSessions) is called during close(). If the connection is already lost, this call may throw an exception that is caught and logged (lines 1543-1548). However, the exception handling is wrapped in a check for the error message 'Connection to MongoDB has been lost', which may not cover all failure scenarios.

Consider broadening the exception handling to catch all exceptions during session cleanup, or validate the connection state before calling endSessions().

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1411c4 and 519528b.

📒 Files selected for processing (3)

• composer.json (1 hunks)
• src/Client.php (12 hunks)
• src/Exception.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/Client.php (1)

src/Exception.php (1)

• Exception (10-214)

🔇 Additional comments (6)

src/Client.php (4)

1551-1553: Calling isConnected() on the client after potential reconnection failure.

At line 1551, $this->client->isConnected() is checked before closing. If the connection was lost and not successfully re-established, this check may fail silently. However, the close() method is designed to clean up regardless, so this is likely acceptable. Just ensure that calling close() on a disconnected client does not throw.

---

1882-1916: Parameter union type for createWriteConcern is broad; validate input thoroughly.

The method signature accepts array|string|int for $writeConcern. The implementation handles each type separately, but there's no validation for unexpected array keys or invalid combinations (e.g., both string and array fields).

The current implementation is adequate for typical use cases, but consider adding validation or documentation to clarify which array keys are supported and which combinations are valid.

If you want to add stricter validation, consider checking for unexpected keys:

+$allowedKeys = ['w', 'j', 'wtimeout'];
+$unexpectedKeys = array_diff(array_keys($writeConcern), $allowedKeys);
+if (!empty($unexpectedKeys)) {
+    throw new Exception('Unexpected write concern keys: ' . implode(', ', $unexpectedKeys));
+}

---

1963-2004: Parameter union type for createReadConcern is appropriate; validate input thoroughly.

The method signature accepts array|string for $readConcern. The implementation handles each type separately and validates the level against a whitelist. The array case also supports afterClusterTime, which is correct for causal consistency.

The implementation is sound.

---

857-883: Cast upsert return to int. query() returns stdClass|array|int and may return a non‐int type; wrap the query() call on line 875 in an explicit (int) cast to guarantee an integer result.

composer.json (1)

26-26: Clarify reason for removing --level=max from PHPStan.

The --level=max flag was removed from the PHPStan check script. This reduces the strictness of static analysis. Was this intentional to address specific issues, or is the plan to gradually increase the level later?

If the removal is temporary, consider documenting the target level in a comment or tracking the upgrade in an issue.

src/Exception.php (1)

194-213: Return type changed from static to self in factory method.

The fromResponse() factory method now returns self instead of static. This prevents subclasses (e.g., ConnectionException, AuthenticationException) from being instantiated via this factory.

Since none of the subclasses (lines 219-259) override fromResponse() or add new constructor parameters, the impact is minimal. However, if future subclasses need to be instantiated via fromResponse(), they will need their own factory methods.

This change is acceptable and improves type safety by making the return type concrete.