Selection: insert() phpDoc covers documented bulk form by haltuf · Pull Request #326 · nette/database · GitHub
Skip to content

Selection: insert() phpDoc covers documented bulk form#326

Open
haltuf wants to merge 1 commit intonette:v3.2from
haltuf:selection-insert-bulk-phpdoc
Open

Selection: insert() phpDoc covers documented bulk form#326
haltuf wants to merge 1 commit intonette:v3.2from
haltuf:selection-insert-bulk-phpdoc

Conversation

@haltuf
Copy link
Copy Markdown

@haltuf haltuf commented Apr 22, 2026

  • bug fix? yes
  • new feature? no
  • BC break? no (phpDoc/tests only)
  • doc PR: see §Docs below

Branched from tag v3.2.9; opening against v3.2 branch per GitHub requirement (tags can't be PR base). Two v3.2 commits ahead are independent — rebase on request.

Regression

Commit 420ebb5 (v3.2.9) narrowed Selection::insert() phpDoc:

- @param iterable|Selection $data
- @return T|array|int|bool
+ @param iterable<string, mixed>|Selection<ActiveRow> $data
+ @return ($data is array<string, mixed> ? T|array<string, mixed> : int)

This rejects the documented bulk form $table->insert([[...], [...]]) (list<array<string, mixed>> is not assignable to iterable<string, mixed>).

Reported on forum: https://forum.nette.org/cs/36954

Runtime check (SQLite, MySQL 8, MariaDB 11, PostgreSQL 16)

Ran insert() across primary-key scenarios on all four drivers. All drivers return the same PHP types (only ID values differ for bulk + autoincrement due to LAST_INSERT_ID semantics):

scenario returns
single-row + autoincrement PK ActiveRow
single-row + manual single-col PK ActiveRow
single-row + multi-col PK (complete) ActiveRow
single-row + multi-col PK (incomplete keys) array (input data)
single-row + no PK int (row count)
bulk + autoincrement PK ActiveRow (first or last inserted, driver-dependent)
bulk + manual single-col PK int
bulk + multi-col PK array (input data)
bulk + no PK int
Selection (insert from query) int

The v3.2.9 conditional return claims int for every non-array<string, mixed> path, but runtime returns array (bulk + multi-col) and ActiveRow (bulk + autoincrement) for two of them on every driver. Runtime has been stable across v3.x.

Fix

  • @param extended by list<array<string, mixed>> (documented bulk form).
  • @return reverted to permissive union T|array<int|string, mixed>|int covering every observed runtime path. Same shape as pre-v3.2.9.
  • One-line summary removed — previous wording ("ActiveRow for single, affected rows otherwise") does not match runtime for bulk + multi-col PK and bulk + autoincrement PK.
  • tests/types/database-types.php: added asserts for single-row, bulk, and from-Selection variants.
  • tests/types/TypesTest.phpt: added TypeAssert::assertNoErrors() so a future narrowing regression fails the test, not just a baseline phpDoc check.

No runtime change.

Note on the extra assertNoErrors() call

TypeAssert::assertTypes() only verifies return types of assertType() calls. An assertType('int', $table->insert($bulk)) passes even on the v3.2.9-regressed code, because the conditional return still evaluates to int when the argument type doesn't match array<string, mixed>. The actual argument.type error on the bulk call site surfaces only through file-wide PHPStan analysis, which assertNoErrors() performs. Without it the test guards nothing against this class of regression.

If you'd rather keep TypesTest.phpt single-purpose (just assertTypes()), I can move assertNoErrors() to a separate .phpt file or drop the call entirely — let me know which you prefer.

Docs

https://doc.nette.org/cs/database/explorer claims "bulk insert returns row count (int)" and "single-row insert without unique PK returns passed data as array", which disagree with the runtime on all four drivers in three cases (bulk + multi-col PK, bulk + autoincrement PK, single + no PK). This PR aligns phpDoc with runtime, not docs. Separate nette/docs PR is in order — happy to open one; wanted to unblock this regression first.

4.0

Same @param regression is present on master; insert() runtime code is byte-identical. Shall I open a parallel PR against master, or cherry-pick?

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.

1 participant