Add event for statement execution failure

Created on 22 December 2023, 6 months ago
Updated 20 February 2024, 4 months ago

Problem/Motivation

In Allow the database query log to be dispatched as log events Fixed 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

Feature request
Status

Fixed

Version

11.0 🔥

Component
Database 

Last updated about 14 hours ago

  • Maintained by
  • 🇳🇱Netherlands @daffie
Created by

🇮🇹Italy mondrake 🇮🇹

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mondrake
  • Merge request !5940Closes #3410476 → (Closed) created by mondrake
  • Status changed to Needs review 6 months ago
  • 🇮🇹Italy mondrake 🇮🇹

    Impacts Introduce a Connection::executeDdlStatement method RTBC .

    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.

  • Status changed to Needs work 6 months ago
  • 🇺🇸United States smustgrave

    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.

  • Status changed to Needs review 6 months ago
  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • Status changed to RTBC 6 months ago
  • 🇺🇸United States smustgrave

    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.

  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • Status changed to Needs review 6 months ago
  • 🇺🇸United States smustgrave

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

  • 🇮🇹Italy mondrake 🇮🇹

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

  • heddn Nicaragua

    The rebase fixing things is explained by 📌 Fix random performance test failures Needs review landing yesterday.

  • Status changed to RTBC 6 months ago
  • 🇺🇸United States smustgrave

    Good find! Thanks for pointing out that ticket.

    • longwave committed 870e5c1c on 11.x
      Issue #3410476 by mondrake, smustgrave: Add event for statement...
  • 🇬🇧United Kingdom longwave UK

    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 record.

  • Status changed to Fixed 5 months ago
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.69.0 2024