Problem/Motivation

In #3313355: Allow the database query log to be dispatched as log events we added events to trace the start of a statement execution and it successful end, but did not cover the case of statement execution failure.

At the moment, a failed statement execution results in no end events being dispatched.

Proposed resolution

Either add another event for execution failure, or change the statement execution end event to also carry the result of the execution.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3410476

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !5940

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
Related issues: +#3384999: Introduce a Schema::executeDdlStatement method

Impacts #3384999: Introduce a Schema::executeDdlStatement method.

For review. This is adding the new event and changing the Statement objects to dispatch it in case of execution failure on the db. I'm also dprecating StatementPrefetchIterator::throwPDOException() which appears to be just dead code right now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record updates

Just in case what do you think about moving the deprecation to a separate ticket? So nothing should be in the way of the new event.

Event makes sense only moved to NW for the change record update and thoughts on question above.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates

Updated the CR and created a new one only for the deprecation. To me, the deprecation belongs here since we are changing the code here, we would not have found it it the code wasn't changed, and we want to ensure the event carries an exception from the underlying connection and not a mock of it.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks for adding that 2nd CR both read fine to me.

Missed event seems good.

Test coverage is proved by test-only feature

1) Drupal\Tests\Core\Database\DatabaseEventsTest::testEventEnablingAndDisabling
Error: Class "Drupal\Core\Database\Event\StatementEvent" not found
/builds/issue/drupal-3410476/core/tests/Drupal/Tests/Core/Database/DatabaseEventsTest.php:43
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/TextUI/Command.php:97
--
There was 1 risky test:
1) Drupal\Tests\Core\Database\DatabaseEventsTest::testEventEnablingAndDisabling
This test did not perform any assertions
/builds/issue/drupal-3410476/core/tests/Drupal/Tests/Listeners/DrupalListener.php:60
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/Framework/TestResult.php:452
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/Framework/TestResult.php:980
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3410476/vendor/phpunit/phpunit/src/TextUI/Command.php:97
ERRORS!
Tests: 4, Assertions: 6, Errors: 1, Risky: 1.

The failing postsql failure is random javascript one.

mondrake’s picture

mondrake
Location 🇮🇹
commented

The failing postsql failure is random javascript one.

I'm not sure it's random, there is a query execution count that maybe needs updating, but I can't see how this MR impacts that. Maybe @catch can have an opinion.

smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Needs review

swear reran it and it passed but failing again, so moving back to NR.

mondrake’s picture

mondrake
Location 🇮🇹
commented

After rebasing, the PostgreSql test failure seems to be gone. Cannot put back to RTBC given I added couple more optimizations.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

The rebase fixing things is explained by #3410450: Fix random performance test failures landing yesterday.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Good find! Thanks for pointing out that ticket.

  • longwave committed 870e5c1c on 11.x 
    Issue #3410476 by mondrake, smustgrave: Add event for statement...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

This cleans up the existing code and makes things more consistent at least from a testing point of view.

Committed 870e5c1 and pushed to 11.x. Thanks!

Also published the change records.

longwave closed merge request !5940

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.