[10.x] In MySQL, harvest last insert ID immediately after query is executed

[piurafunk]

I work on an app whose database is MySQL, and its sql_mode is set to ''; the primary impact of this is that "strict mode" is off, and too long/large data is truncated and inserted, instead of rejected. As you can imagine, this leads to problems later on, when we try to query that truncated data and we're missing pieces, or an integer/decimal is set to a max/min value.

In an attempt to log the warnings that are emitted from the DB in these scenarios, I set up a listener for QueryExecuted, and called SHOW WARNINGS to get the list of warnings, and report them to our error reporting system. This allows us to see the warnings, in the context of the executing query and the PHP stack trace, without crashing our app. We hoped to work these warnings down to an acceptable level, then turn on strict mode.

However, I discovered that executing any queries during the QueryExecuted event causes PDO to drop the last insert ID, and so all tests that insert any data broke, due to getting '0' back from PDO, rather than the actually generated ID. After some Googling, I found this PHP bug report, that claims this is not a bug, but the intended behavior:

From what I understand this is expected behavior, so mysqlnd is backwards compatible with libmysql. The core developers purposefully reset the last insert ID on non INSERT statements to emulate the same behavior as the old libmysql library.

In order to still fetch the last insert ID, but also be able to execute queries during the QueryExecuted event, I've modified the MySQL specific Processor and Connection classes. The test added fails without the changes applies.

---

It is possible to accomplish this without framework changes, because we can extend the MySQL classes, and register a customer driver via \Illuminate\Database\Connection::resolverFor() and \Illuminate\Database\Connection::getResolver(). In fact, we will probably be doing that until this is merged and we can update to the version its released with. However, I feel it is beneficial to add it to the framework anyway. The other PDO compatible drivers don't seem to have this problem. I did some local testing with PostgreSQL, and it behaved fine without any additional changes: I could execute queries during QueryExecuted and the last insert ID was still harvested correctly.

I acknowledge that my use case is niche, but this should help any future devs who happen to need to execute queries during that event.

---

I'm targeting 10.x because it's just barely still in "bug support" dates, and this does seem like a bug. I'm happy to rebase to 11.x if it's considered a minor feature, or 12.x, due to the change in signature to \Illuminate\Database\MySqlConnection::insert(). I think that change would affect devs who extend MySQLConnection.

[GrahamCampbell]

Great spot.

[decadence]

Related:
laravel/docs#9091

[daftspunk]

Hey @piurafunk,

I think that change would affect devs who extend MySQLConnection.

Just a heads up, this change appears to affect devs who extend MySQLConnection. We have a customer reporting issue with the following composer upgrade path:

• Upgrading laravel/framework (v10.48.15 => v10.48.19)

Here is the related stack trace:

BadMethodCallException: Method October\Rain\Database\Connections\MySqlConnection::getLastInsertId does not exist. in ~/vendor/laravel/framework/src/Illuminate/Macroable/Traits/Macroable.php:112
Stack trace:
#0 ~/vendor/laravel/framework/src/Illuminate/Database/Query/Processors/MySqlProcessor.php(37): Illuminate\Database\Connection->__call('getLastInsertId', Array)
#1 ~/vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php(3507): Illuminate\Database\Query\Processors\MySqlProcessor->processInsertGetId(Object(October\Rain\Database\QueryBuilder), 'insert into `fr...', Array, 'id')
#2 ~/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(1982): Illuminate\Database\Query\Builder->insertGetId(Array, 'id')
#3 ~/vendor/october/rain/src/Database/Builder.php(249): Illuminate\Database\Eloquent\Builder->__call('insertGetId', Array)
#4 ~/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1334): October\Rain\Database\Builder->__call('insertGetId', Array)
#5 ~/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1299): Illuminate\Database\Eloquent\Model->insertAndSetId(Object(October\Rain\Database\Builder), Array)
#6 ~/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1138): Illuminate\Database\Eloquent\Model->performInsert(Object(October\Rain\Database\Builder))
#7 ~/vendor/october/rain/src/Database/Model.php(383): Illuminate\Database\Eloquent\Model->save(Array)
....

Related extended classes:

• October\Rain\Database\Connections\MySqlConnection (patch added)
• October\Rain\Database\Builder

[piurafunk]

From what I can see, your Connection class extends the Illuminate Connection class, rather than MySQLConnection. Yours is named MySQLConnection, but doesn't actually extend from the Illuminate MySQLConnection. However, your MySQLConnection does use the default MySQLProcessor, which does have these changes. I'm not sure if it's worth it for you, but maybe extending the Illuminate MySQLConnection would remove the need for your patch.

[daftspunk]

Ah, yes. You're right, the fragility here could be by our own design. The reason we don't extend the Illuminate MySQLConnection base class, since we also require extending the base Illuminate\Database\Connection class to swap the QueryBuilder instance and other things. Perhaps we could overcome this by using a trait instead. Thanks for your reply.