🇮🇹Italy @mondrake

quietone → credited mondrake → .

The more I look at this, the more I think we should build on top of 📌 Move the on-demand-table creation into the database API Needs work , and introduce a generic method that takes care of making db calls 'safe' from different perspectives.

For example, what we have in that issue at the moment is a ::executeEnsuringSchemaOnFailure that in case of failure of a db operation, tries to build missing tables and retries the db operation:

    $execution = $this->connection->executeEnsuringSchemaOnFailure(
      execute: function (): int {
        return do_something();
      },
      schema: $schemaDefinition,
      retryAfterSchemaEnsured: TRUE,
    );

we could make that more generic:

    $execution = $this->connection->safelyExecute(
      execute: function (): int {
        return do_something();
      },
      withSchema: $schemaDefinition,
      withReconnection: TRUE,
      retryOnFailure: TRUE,
    );

rebased

The motivation is clear and sensible, +1.

However, pinging with a query anytime the client collection is fetched means a lot of db roundtrips, and as such kill performance - if I am not mistaken, getClientConnection() is called anytime a transaction is opened. BTW, that would also mean that non-transactional statement executions will not be covered.

I think it would be preferable to store the client connection state in the Connection object, being 'active' at opening, then through exception handling change its state to 'connection lost' when an issue occurs. Code executing calls to the db could then check that state before executing and ping/close/reconnect in case. A challenge for sure would be how to determine a clear connection lost exception vs other exceptions.

Another aspect to take care of is understanding the implications of a reconnect occurring while a db transaction is open - is the db transaction rolled back, how will the db failure reflect in the transaction manager, etc.

On the positive side, we could think of building on the experience from 📌 Move the on-demand-table creation into the database API Needs work and think of a method that could embed a retry of executing a statement, i.e. in case of failure for disconnect, reconnect and retry.

Finally - MySql (and possibly PgSql) would lose the connection, but what should be the behavior for Sqlite? It also can fail if the file is not in the same filesystem of the web server.

@nicxvan thanks, I like where MR 10607 is heading, especially in the part where the early subscribers to be registered with the early dispatcher are moved to the database API.

However, this is creating an (early) event dispatcher that is attached to the Connection class instead of being unique across the request. Not a big deal for now, right, but what if in the future more code would need to dispatch events early? Cache for instance may want to dispatch an event to be responded by a database listener (again, not the case know, but to prepare for possible future). I'd rather keep a singleton somewhere outside of the Connection class, so that we do not end up with vertical dispatchers not talking to each other. That's an architecture topic, tagging so we can have a review and guidance.

Then, I think we should use a closure also to register the early subscribers in place of the const array - so to allow passing instantiated objects that may require parameters (atm in both MRs the assumption is that the subscribers have no parameters in the construction).

Tests are failing.

Thanks all!

Please do not post patch files: this project only accepts MRs.

Automated tests are failing, so this issue needs work to solve the failures.

Green on all three core supported databases! Ready for review.

I am bumping to Major as it's known there are problems with the current handling of autocommit in HEAD.

Updated title and IS

The PHPStan job is failing, because the new return type introduced makes the added entry on the PHPStan baseline no longer needed. Needs to remove that entry (or regenerate the baseline).

@daffie thanks, if you do not mind I'd leave this to NR a bit longer to see if more reviews/input come. I will definitely do the CR and IS updates when we have settled on the final solution.

@nicxvan I am not sure I understand; feel free to open another issue branch and MR if you want to propose an alternative!

Restored bot happiness. Back to NR.

Thanks for review @daffie!

#22 would be nice, but I do not see it as a blocker.

Addressed the comments.

Looks great to me, thanks for hearing my question. I triggered Sqlite and PgSql pipelines for safety, even though I do not think they are relevant here. RTBC

Looking at the motivation, I am not sure about this. The file must have a size, but we cannot get it so we hide the problem. BTW possibly the image cannot be loaded by GD either. IMHO the right solution here would be to download the file from S3 to local storage, get its stats and use it for manipulation. IIRC the ImageMagick toolkit does something similar since it does not support stream wrappers.

mondrake → created an issue.

Question: can't we use something from Drupal\core_fake\Driver\Database\CoreFakeWithAllCustomClasses instead of adding a completely new dummy db and all of its minimal implementations?

Anyway, the above should not hold this. RTBC

📌 Allow parsing and writing PHP constants and enums in YAML files Needs work would help, we could pass an enum parameter in the service definition file, which we cannot presently.

😀

Added 2 CR stubs, one [#3494043] for the generic availability of the event dispatcher before container bootstrap, and one [#3494044] for its usage in Database::Connection. Will complete at later stage, doing at this stage has an high risk of getting stale information in it while reviews and tuning still needs to happen.

Just noticed the CR links redirect to this issue and not to a CR. A CR for this is actually missing.

#16 is addressed, back to RTBC

Let’s see if we can make one more step and make the factory a decorator of the event dispatcher.

Nice! LGTM now. Triggered tests for SQLite and PgSql.

Added some docs inline.

Postpone on 📌 Explore injecting the event dispatcher in the database service Active

few more nits commented inline

@nicxvan re #5 this is about the event dispatcher itself, not about the subscribers.

My 2 cents:

- minor is important for knowing exactly since when new code should be used, for the reasons explained ^^. Patch is not relevant here.
- major is enough for removal

so I'd suggest

@deprecated in drupal:11.1 and is removed from drupal:12. %extra-info%.
@see %cr-link%

https://github.com/mglaman/phpstan-drupal/releases/tag/2.0.0 is out, supporting PHPStan 2. This is unblocked now.

This is now a duplicate of 🐛 Ensure run-tests.sh and PHPUnit CLI run with the same list of tests to be executed Active .

mondrake → changed the visibility of the branch 3492391-explore-injecting-the to hidden.

Ooops no, there's no 4.0.x branch... :)

Thanks! Does this need to be applied to 4.0.x too?

Thank you! We need to fix the phpdocs, too.

@daffie well, that's why I suggest using events, that gives us the possibility to extend/override how they are processed. At least conceptually, the MongoDB driver could implement a subscriber/listener for the ExecuteMethodEnsuringSchemaEvent, make it so that it processes the event before core's SchemaRequestSubscriber does, do its alternative processing of the schema request, and stop propagation of the event so that core no longer processes it afterwards. Of course, needs to be proven in practice.

Filed 📌 Explore injecting the event dispatcher in the database service Active as a possible follow up.

mondrake → created an issue.

Now Flood\DatabaseBackend implemenrs #166, and if this is OK we need to convert the rest where possible (i.e. calls to dynamic queries extending Query).

Pausing now to let reviewing happen.

MR!10403 is now doing #163, deprecates/converts all uses of ::ensureTableExists() and ::catchException(), and is green on all databases. A review at this stage would be helpful.

#166 is just a piece of cake away with this underlying layer, so I will push that forward.

#24 is above me, but to me it sounds like it can be a follow up anyway.

Thanks for review @daffie! Let's continue with the new approach then. Before completing the conversions: add tests for Connection::executeEnsuringSchemaOnFailure().

mondrake → changed the visibility of the branch 2371709-lazy-table-creation-trait to hidden.

I think this should go in the upcoming point releases - it fixes a bug introduced by a MR not released yet in 10.4 and 11.1, and the bug leads to data corruption.

MR!10403 is for review. That includes the framework and some conversion, but not all of them. Once/if the framework is agreed, we can easy complete the remaining ones.

An interesting next step would be to pass the schema as a value object instead of an array, but there are other issues for that.

mondrake → changed the visibility of the branch 2371709-use-events-II to hidden.

#166 would have BC concerns (db drivers would have to match the behavior of the onFailureEnsureSchema() method on all classes extending from Query), so maybe for now we can settle on a proxy of that.

Hm... looking at the MR now, I start figuring out we could simplify further, and let the dynamic query layer handle this:

  public function getId(): int {
        return $this->connection->insert('batch')
          ->fields([
            'timestamp' => $this->time->getRequestTime(),
            'token' => '',
            'batch' => NULL,
          ])
          ->onFailureEnsureSchemaAndRetry([static::TABLE_NAME => $this->schemaDefinition()])
          ->execute();
  }

  public function delete($id) {
      $this->connection->delete('batch')
        ->condition('bid', $id)
        ->onFailureEnsureSchema([static::TABLE_NAME => $this->schemaDefinition()])
        ->execute();
  }

The move of locations broke 🐛 Ensure run-tests.sh and PHPUnit CLI run with the same list of tests to be executed Active - which is correct. Rebased that.

A PoC for #163 in MR!10397.

This is very nice regardless of MongoDb in its enforcing proper dependencies of actions.

Is it possible to add a test for the new subscriber? More comments inline in the MR.

A bit late to the party... but why not to use events instead of a trait? ✨ Use events in Database Schema operations Active

For example (conceptual, not tested):

from

  /**
   * Returns a new batch id.
   *
   * @return int
   *   A batch id.
   */
  public function getId(): int {
    $try_again = FALSE;
    try {
      // The batch table might not yet exist.
      return $this->doInsertBatchRecord();
    }
    catch (\Exception $e) {
      // If there was an exception, try to create the table.
      if (!$try_again = $this->ensureTableExists()) {
        // If the exception happened for other reason than the missing table,
        // propagate the exception.
        throw $e;
      }
    }
    // Now that the table has been created, try again if necessary.
    if ($try_again) {
      return $this->doInsertBatchRecord();
    }
  }

to

  /**
   * Returns a new batch id.
   *
   * @return int
   *   A batch id.
   */
  public function getId(): int {
    $event = new EnsureTableExistEvent(
      callable: [$this, 'doInsertBatchRecord'],
      table: static::TABLE_NAME,
      schema: $this->schemaDefinition(),
    );
    $this->connection->dispatchEvent($event);
    return $event->result();
  }

OR

from

  public function load($id) {
    // Ensure that a session is started before using the CSRF token generator.
    $this->session->start();
    try {
      $batch = $this->connection->select('batch', 'b')
        ->fields('b', ['batch'])
        ->condition('bid', $id)
        ->condition('token', $this->csrfToken->get($id))
        ->execute()
        ->fetchField();
    }
    catch (\Exception $e) {
      $this->catchException($e);
      $batch = FALSE;
    }
    if ($batch) {
      return unserialize($batch);
    }
    return FALSE;
  }

to

  public function load($id) {
    // Ensure that a session is started before using the CSRF token generator.
    $this->session->start();
    try {
      $batch = $this->connection->select('batch', 'b')
        ->fields('b', ['batch'])
        ->condition('bid', $id)
        ->condition('token', $this->csrfToken->get($id))
        ->execute()
        ->fetchField();
    }
    catch (\Exception $e) {
      $event = new TablePossiblyMissingEvent(
        exception: $e,
        table: static::TABLE_NAME,
        schema: $this->schemaDefinition(),
      );
      $this->connection->dispatchEvent($event);
      $batch = FALSE;
    }
    if ($batch) {
      return unserialize($batch);
    }
    return FALSE;
  }

Added deprecation and draft CR.

Let's add deprecation for StatementPrefetchIterator::fetchColumn. On it.

Fixed and added test coverage.

Rebased after commit of 🐛 StatementPrefetchIterator::fetchField fails silently if the column index is out of range Active . I swapped responsability between ::fetchColumn() and ::fetchField() in StatementPrefetchIterator, very minor but I think this requires to be set back to NR. Sorry.

rebased, still green.

@daffie this test will work on a vanilla install; if you have custom/contrib modules, it could very well be that there are tests that PHPUnit cannot discover (see the IS point 2). In any case, could you rather share PHPUnit's output than the dump?

Added 📌 Check StatementPrefetchIterator::fetchCol after strenghtening of ::fetchField Active as a followup for #16, and replied inline referencing existing issues to deprecate StatementPrefetchIterator::fetchField.

mondrake → created an issue.

@daffie have you checked your local phpunit.xml vs the one in the MR?

Ok, so empty resultset prevails on missing column index in PDO, and prefetch works the same. I think we are covered now.

Interestingly, trying to insert FALSE in a nullable database field behaves differently between MySql and SQLite - MySql fails hard at insert, SQLite inserts something that is returned as an empty string when reading back.

Working on a test case that can be db abstract.

added one more test case

fixed docs and added a test

TL;DR

• returning a string -> valid value
• returning FALSE -> no more rows in resultset
• throw exception -> invalid column index

Full story:

fetchField() is historically an alias of \PDOStatement::fetchColumn() - it is so since #225450: Database Layer: The Next Generation → .

So IMHO we should make a non-PDO implementation behave as closely as possible to its original.

Now, \PDOStatement::fetchColumn() documentation says (emphasis mine)

Returns a single column from the next row of a result set or false if there are no more rows.

returning false is btw a potential issue, as the same documentation makes clear:

... fetchColumn() should not be used to retrieve boolean columns, as it is impossible to distinguish a value of false from there being no more rows to retrieve

However, this is mitigated in Drupal by the fact that all values returned from the statement are strings, by setting \PDO::ATTR_STRINGIFY_FETCHES in the connection options.

So, the fetch will return a PHP string if a value is available (independently from its db representation), and a PHP bool === false if no rows are available.

If the required column index is out of range, \PDOStatement::fetchColumn() throws a \ValueError (it's not documented, but the test here proves that). This makes sense because if we were returning a bool false instead, we would be again unable to distinguish between no more rows and no column defined, which are very different situations - no more rows is a legitimate runtime state, whereas a wrong column index means most likely the query was built wrong. I think in the emulated fetch of the prefetched data we should do the same.

Will look into this, thanks

@daffie MySql and PostgresSql are throwing \ValueError today, so the 3 core drivers behave differently from each other ATM.

It seemed to me that aligning SQLite would be the best option given that anyway it's more unlikely to be used in production.

mondrake → created an issue.

Looks good.

By the time of this, PHPUnit will likely be on its version 13, and close to 14 - if they keep the cycle of 1 major per year.

#27 when there will be a PHP 8,5 docker image available, we can easily add it to the job matrix here, allowing failures. That will help showing deprecations for 8.5 while keeping the entire pipeline green.

mondrake → created an issue.

Found a glitch while working on the follow up.

mondrake → created an issue.