Move the on-demand-table creation into the database API

Tried to Re-roll patch from #120 📌 Move the on-demand-table creation into the database API Needs work to11. Branch.

Reroll seemed to have test failures.

I have updated the IS and created a CR.

daffie → changed the visibility of the branch 2371709-move-the-on-demand-table to hidden.

The Mr is ready for a review.

Oh I've had my eye on this it will make hook schema deprecation much nicer.

added suggestion

@andypost: Thank you for the review.

Changing the class variable to a readonly variable and setting its value is not allowed in PHP. From the PHP documentation:

Specifying an explicit default value on readonly properties is not allowed, because a readonly property with a default value is essentially the same as a constant, and thus not particularly useful.

See: https://www.php.net/manual/en/language.oop5.properties.php

I have changed the variables to be of the string type.

Out of interest why could we not use constants for this case? The classes should be able to override those.

@catch: I some cases it can be a constant in other cases it cannot. Like with Drupal\Core\Menu\MenuTreeStorage, Drupal\Core\Routing\MatcherDumper and Drupal\Core\KeyValueStore\DatabaseStorage.

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;
  }

A PoC for #163 in MR!10397.

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();
  }

#163-#166 are incredibly concise so if we can make those work, that is very tempting.

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

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

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-lazy-table-creation-trait to hidden.

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

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.

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.

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

@mondrake: It looks like a beautiful solution. My problem is that the solution will not work with MongoDB. The solution relies on that the database will throw an exception when the table does not exists. However that is not what MongoDB does. MongoDB just creates a table when one does not exists on an insert. A table in MongoDB is called a collection. It will be a table with no validation added to make sure that it only allows data in the right form being added. See: https://www.mongodb.com/docs/manual/reference/method/db.collection.inser....

The reason I started to work on this issue was to add a hook to allow the database driver to change the table schema. The database driver for MongoDB stores timestamps as MongoDB\BSON\UTCDateTime instead of an integer value as is done by the by core supported databases.

If we could make it to work for MongoDB that would be great. I will need some time to think how I can make it to work with MongoDB. If you have some ideas or suggestions, then please speak up.

Wrongly assigned to issue to @acbramley

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

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