Skip to content

PHPC-2486: Require non-null namespace and database arguments in Manager#1963

Merged
alcaeus merged 1 commit intomongodb:v2.xfrom
alcaeus:phpc-2486-require-non-null-params
Mar 27, 2026
Merged

PHPC-2486: Require non-null namespace and database arguments in Manager#1963
alcaeus merged 1 commit intomongodb:v2.xfrom
alcaeus:phpc-2486-require-non-null-params

Conversation

@alcaeus
Copy link
Copy Markdown
Member

@alcaeus alcaeus commented Mar 26, 2026

PHPC-2486

I also audited Server.c to confirm that it uses Z_PARAM_STRING in all the right places.

@alcaeus alcaeus requested a review from a team as a code owner March 26, 2026 12:12
@alcaeus alcaeus requested review from Copilot and paulinevos and removed request for a team March 26, 2026 12:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aligns MongoDB\Driver\Manager’s internal parameter parsing with its public API contract by disallowing null for database ($db) and namespace ($namespace) arguments in the various execute* methods.

Changes:

  • Require non-null database strings for executeCommand, executeReadCommand, executeWriteCommand, and executeReadWriteCommand.
  • Require non-null namespace strings for executeQuery and executeBulkWrite.
Comments suppressed due to low confidence (1)

src/MongoDB/Manager.c:292

  • This change tightens parameter parsing (db/namespace no longer accepts null). There are many PHPTs covering these execute* methods, but none asserting the new failure mode for null inputs; adding regression tests for executeCommand/executeReadCommand/executeWriteCommand/executeReadWriteCommand and executeQuery/executeBulkWrite with a null first argument would help prevent accidental reintroduction (or potential segfaults if arginfo/type checks are bypassed).
	PHONGO_PARSE_PARAMETERS_START(2, 3)
	Z_PARAM_STRING(db, db_len)
	Z_PARAM_OBJECT_OF_CLASS(command, phongo_command_ce)
	Z_PARAM_OPTIONAL
	Z_PARAM_ZVAL_OR_NULL(options)
	PHONGO_PARSE_PARAMETERS_END();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@paulinevos paulinevos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you audited usages, but these are also covered by tests? Do we check coverage in CI actually?

@alcaeus alcaeus merged commit 7b030ed into mongodb:v2.x Mar 27, 2026
47 checks passed
@alcaeus
Copy link
Copy Markdown
Member Author

alcaeus commented Mar 27, 2026

I know you audited usages, but these are also covered by tests? Do we check coverage in CI actually?

Nope, we don't have any tests for null parameters. At the moment we don't have any coverage information, I'd like to add that eventually.

@alcaeus alcaeus deleted the phpc-2486-require-non-null-params branch March 27, 2026 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants