[nadorator]

Hello!

• Type: bug fix
• Link to issue: [BUG]: Model keeps a default id of 0 after insert on MySQL since 5.12 (lastInsertId() skipped) #17689

In raising this pull request, I confirm the following:

• I have read and understood the Contributing Guidelines
• I have checked that another pull request for this purpose does not exist
• I wrote some tests for this PR
• I have updated the relevant CHANGELOG
• I have created a PR for the documentation about this change

Small description of change:

Since #16920 (fix for #16436), Model::doLowInsert() reuses any non-empty identity value instead of calling lastInsertId(). On MySQL, 0 is not an explicit id: MySQL replaces it with the next auto-increment value, so a model whose identity property defaults to 0 keeps 0 after save() / create().

The value on the model is now reused on adapters with sequences (as before), and for any explicit non-zero value (same value and type as before). Only 0 on adapters without sequences goes through lastInsertId() again. The PostgreSQL behaviour from #16436 is unchanged, and the explicit-id path keeps returning the value as set (no int to string change). supportSequences() is now called once and stored in useSequences.

Tests (tests/database/Mvc/Model/CreateTest.php):

• testMvcModelCreateWithZeroIdentityMysql (data set 0 and '0'): the model gets the id stored in the table.
• testMvcModelCreateWithExplicitIdMysqlSqlite: an explicit 77 is kept as int 77.

Checked locally (Debian 12, PHP 8.2.35, MariaDB 10.11.19, PostgreSQL 16, SQLite), with one extension built from 5.0.x and one from this branch (Zephir 1.6.1):

Suite	5.0.x	this branch
CreateTest mysql	2 failures (the new zero-identity data sets)	OK (5 tests)
CreateTest sqlite	OK (3 tests)	OK (3 tests)
CreateTest pgsql, including the #16436 tests	OK (4 tests)	OK (4 tests)
tests/database/Mvc/Model mysql / sqlite / pgsql	same results as this branch except the 2 failures above	no new failure

The remaining errors in the full tests/database/Mvc/Model runs were the same on both builds and came from my environment (APCu not enabled in CLI). phpcs and php-cs-fixer --dry-run pass on the test file.

Thanks

🤖 Generated with Claude Code

[niden]

@nadorator Thank you for this PR.

I passed this through the QA tool I have and the findings are below which I verified:

1. The code comment is not correct for SQLite (phalcon/Mvc/Model.zep:4907-4908): "Without sequences, 0 is not an explicit value" is false on SQLite, which stores 0 as given. It is also false on MySQL with NO_AUTO_VALUE_ON_ZERO. The code is still correct, because lastInsertId() returns the stored id in all these cases. Fix: say that, for example "Without sequences, lastInsertId() returns the stored id. MySQL can replace 0 with a generated id."

2. The SQLite 0 case is not tested: after this change, an id of 0 on SQLite comes back as the string "0" from lastInsertId(). Before, it stayed as the int 0. The value is the same but the type changes, and no test covers it.
   Fix: add the sqlite group to the zero-identity test with a separate assertion, or add a small SQLite-only test.

3. Union type spacing (tests/database/Mvc/Model/CreateTest.php:226): int | string does not match the repo style, int|string|null (tests/support/Migrations/InvoicesMigration.php:27). Fix: int|string $zero.

I think you can run composer cs-fixer-fix and that should correct it but I don't remember. I might have to double check the rules for that. Either way small nit, fine if you leave it like that - I will work on automating more our code style

4. The CHANGELOG entry is longer than needed (CHANGELOG-5.0.md:25): the second clause ("0 is no longer reused…") repeats the first.
   Fix: stop after "…when the identity property is 0."

5. Loose assertion (tests/database/Mvc/Model/CreateTest.php:242): assertEquals hides the type of the id. This is acceptable because the PDO fetch type differs between drivers. It is optional to also assert (int) $invoice->inv_id === (int) $stored->inv_id with assertSame.

The ext folder has not been generated so that will cause an issue when sending this to v5. Don't worry about that, I will regenerate it and push. Posting this here as an FYI.

Final question, are you planning on issuing a mirror PR for v6 or should I do it? Fine with either answer.

[niden]

For 3 I got carried away. cs-fixer works only on cphalcon since it cannot handle zep files so ignore that.