Exclude read concern commands

[shimonewman]

Summary by CodeRabbit

• Bug Fixes
  • Improved read-concern handling for commands that don’t support it (notably getMore and killCursors), ensuring readConcern is correctly removed so these commands run reliably both inside and outside transactions.
  • Minor test formatting cleanup with no behavior changes.

[coderabbitai]

Walkthrough

Adds public constant COMMAND_KILL_CURSORS = "killCursors" to Client. Adds private array property $readConcernNotSupportedCommands initialized with [self::COMMAND_GET_MORE, self::COMMAND_KILL_CURSORS]. Updates query() so readConcern is removed when either the command has a txnNumber without startTransaction and includes readConcern, or the command’s first key is a member of $readConcernNotSupportedCommands. Removes trailing blank lines in several tests in tests/TransactionTest.php. No other API changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

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 pull request title "Exclude read concern commands" directly aligns with the primary change in the changeset. The main modification adds logic to identify and exclude readConcern from commands that don't support it (GET_MORE and KILL_CURSORS), which is exactly what the title describes. The title is concise, specific, and clearly conveys the intent without unnecessary details or vague phrasing. It accurately reflects the core objective of the pull request from the developer's perspective.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch exclude-read-concern-commands

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce62464 and 5206ea9.

📒 Files selected for processing (1)

• src/Client.php (3 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Tests

🔇 Additional comments (3)

src/Client.php (3)

69-69: LGTM! Constant definition follows existing conventions.

The COMMAND_KILL_CURSORS constant is correctly defined and consistent with other command constants in the class.

---

100-106: Excellent! Previous issues have been resolved.

The property definition is now correct:

• Property name is properly spelled as readConcernNotSupportedCommands
• Doc comment is clean and accurate
• The array correctly identifies MongoDB commands that don't support readConcern options (getMore and killCursors)

---

329-341: LGTM! The conditional logic is now correct and well-structured.

The previous syntax errors and formatting issues have been properly fixed:

• Single, well-formed if statement (no duplicate if)
• Clean formatting with no extra whitespace
• Correct property name reference

The logic correctly removes readConcern in two scenarios:

1. Non-first operations in a transaction (existing behavior)
2. Commands that don't support readConcern options (new behavior)

The implementation using array_key_first() with in_array() is appropriate and handles edge cases safely with the null coalescing operator.

---

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: 2

🧹 Nitpick comments (1)

src/Client.php (1)

100-106: Consider adding test coverage for readConcern exclusion logic.

The new exclusion list prevents readConcern from being applied to getMore and killCursors commands. Since this is a behavioral change in how the client handles readConcern, consider adding tests to verify:

1. Commands in the exclusion list don't receive readConcern even when causal consistency is enabled
2. Other commands still properly receive readConcern when appropriate
3. The exclusion works correctly both inside and outside transactions

Run the following script to check current usage of these commands:

#!/bin/bash
# Description: Find usage of getMore and killCursors to understand test coverage needs

# Search for getMore usage
echo "=== getMore usage ==="
rg -n --type=php -C3 'COMMAND_GET_MORE|getMore\(' 

# Search for killCursors usage  
echo "=== killCursors usage ==="
rg -n --type=php -C3 'COMMAND_KILL_CURSORS|killCursors'

# Check test files
echo "=== Test files ==="
fd -e php test

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ecfad6a and 62d03ae.

📒 Files selected for processing (2)

• src/Client.php (3 hunks)
• tests/TransactionTest.php (0 hunks)

💤 Files with no reviewable changes (1)

• tests/TransactionTest.php

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Tests

🔇 Additional comments (1)

src/Client.php (1)

69-69: LGTM! Constant addition is correct.

The new COMMAND_KILL_CURSORS constant follows the established pattern and correctly represents MongoDB's killCursors command.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/Client.php (1)

100-106: Fix typo in property name and comment.

The property name contains a typo: readConcernNotSupporteedCommands should be readConcernNotSupportedCommands. Additionally, the comment has an extra space between "support" and "readConcern".

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62d03ae and ce62464.

📒 Files selected for processing (1)

• src/Client.php (3 hunks)

🧰 Additional context used

🪛 GitHub Actions: CodeQL

src/Client.php

[error] 333-333: PHPStan: Syntax error, unexpected T_IF on line 333.

🪛 GitHub Actions: Linter

src/Client.php

[error] 3-3: Parse error: syntax error, unexpected token "if" on line 3. This occurred during linting (Pint) after running Composer install.

🪛 PHPStan (2.1.31)

src/Client.php

333-333: Syntax error, unexpected T_IF on line 333

(phpstan.parse)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Tests

🔇 Additional comments (1)

src/Client.php (1)

69-69: LGTM!

The new constant follows the established naming convention and correctly defines the killCursors command.