READ COMMITTED requirement check doesn't account for database views

Bumping this to major. This is very confusing for site owners. Would be great if we could fix this before Drupal 10.1 is released. Maybe this should even be a "critical" one.

The issue only applies to MySQL/MariaDB views. Right?

Yes

@chi you are right.

I think it would be reasonable to restrict ::findTables() to only finding actual database tables - we could do that with a change record probably.

I tried to think of bc-friendly options (adding a second argument for $include_views = TRUE? adding a settings override?) but couldn't come up with a use case.

I guess this just needs someone to write a patch?

Here's a patch to get things started. This will:

1. Add an optional parameter to findTables() to exclude SQL views from the query.
2. Use that parameter in the mysql_requirements() call in mysql.install.

The patch works correctly on a 10.1.6 drupal site and stops showing warnings against views in the database.

Confirming this patch worked for me on 10.1.6 as well.

Confirm this works for me too.

Confirming that this worked OK with 10.2.

Patch at #8 no longer works with v11.2
It seems that core/modules/mysql/mysql.install has been replaced by core/modules/mysql/src/Hook/MysqlRequirements.php

This patch seems to work OK with v11.2: 3362726-9.exclude-views-from-table-list-11.2.patch

Can you convert this to an MR?

Tests don't run against patches.

Test coverage might involve:
https://git.drupalcode.org/issue/drupal-3362726/-/blob/3362726-read-comm...

Added new test function.

Did you accidentally remove the actual change? I only see the test stub.

I havent applied any of the patches yet. It needs to be done.

findTables() has a new boolean parameter. The test would be setting the param to true in order to exclude views and allow only tables.

The test will break against the current codebase because it will be setting a non-existent parameter. The code within the function is not going to be tested because the test wont reach it?

Any ideas how such issues are correctly addressed in a kernel test?

Okay, I see that the errors in the IS are triggered because views do not need to contain a primary key. There is existing code in other test functions involving the presence/ non-presence of primary keys. Possibly test coverage should be by extending such functions. Or perhaps can incorporate the primary key test code into the new test function..

If test table (view) contains no primary key, check if findTables() 2nd parameter is set to TRUE. If it is, check if the table is a view. Assertion passes if it is a view with no primary key..

The core Schema.php has a method createTable() but has no equivalent method to create views. The tests rely on createTable(). Not sure it is worthwhile or feasible to support views in the piecemeal fashion of this issue.

If we are going to take cognisance of views should be not build it into the database api including the core Schema.php. As it stands we will need, i think, to use raw sql queries in the tests ie CREATE VIEW statements as mentioned in the steps to reproduce.

If we could add minimal support for DB views into core say by adding createView() method, possibly related CRUD methods too. Then do minor refactor of existing test functions eg the one that tests tables all have primary keys. So the test function would do a 'it has not got a primary key. Is it a view? Yes? In which case, no problem.'

I think for this issue we would manually run asql command to create a db view on the test db.

Creating an api for creating and managing db views is it if scope here and not necessary.

I don't know how we go about doing my recommendation here, just giving thoughts on approach and scope.

I don't think we should change the default behaviour of findTables()... if we do we'll break any tests for contrib or custom code which are creating a view at the moment as we rely on findTables() to clear up test databases. Maybe we can introduce a flag that defaults to the current behaviour but allows you to optionally list only real tables or only views.

That said I guess

    if ($original_prefix != $test_prefix) {
      $tables = Database::getConnection()->schema()->findTables('%');
      foreach ($tables as $table) {
        if (Database::getConnection()->schema()->dropTable($table)) {
          unset($tables[$table]);
        }
      }
    }

is flawed because dropTable is not going to work... and looking at our dropTable() implementation

  /**
   * {@inheritdoc}
   */
  public function dropTable($table) {
    if (!$this->tableExists($table)) {
      return FALSE;
    }

    $this->executeDdlStatement('DROP TABLE {' . $table . '}');
    return TRUE;
  }

Well the tableExists call is going to return false... so it's not going to even error.

Also I think both SQLite and Postgres are unaffected because their implementations of findTables() is different.

So... tldr; perhaps fixing findTables() to only return tables for MySQL is actually the way to go here!

Just added an MR that shows that the MySQL driver is on it's own here by returning views from findTables()... so I think we're okay just to remove the capability from MySQL. Discussions around a database views API can happen elsewhere.

My MR now also includes a fix that's limited to MySQL (and MySQLi) where the problem occurs.

alexpott → changed the visibility of the branch 3362726-read-committed-requirement to hidden.

@alexpott

I added code comment to: https://git.drupalcode.org/project/drupal/-/merge_requests/12442/diffs?c...

Is that commit necessary for filtering views amongst tables?

+1 #27

#27 is wrong. See #28 - if your module creates a view it'll error on trying to clean up a test in Postgres - not sure what it'll do for MySQL but the drop table is certainly not going to clean up the table as expected. All of our code is assuming findTable() only returns tables - which is excatly how it works for Postgres and SQLite... it just doesn't work this way for MySQL. I would argue that findTables() shoudl only find tables because of this and that the current MySQL behaviour is the outlier. The tests and changes added here prove this.

@oily the MySQL 5 thing is altered so that getComment() has a consistent return value for both MySQL and Postgres. FWIW the MySQL implementation has no business changing the FALSE to an empty string because of the preg_replace.

@alexpott The commit I queried.. I understand the purpose now. Line 688 reveals the relevance to this issue. If i had scrolled up a bit I would have seen it.

Test coverage in place. Test-only test fails as it should.

Can move to RTBTC?

@oily yes you can.

I created the CR as per #6

Reviewed CR. 'materialized' views is interesting. Just looked it up. Need to research further. Asking myself 'how to create material view as opposed to a normal view..' Could CR benefit from example Mysql query to create a material view? The example is the IS is: "CREATE VIEW test AS SELECT * FROM users;" That does create a 'material' view?

@oily it's just what they are called - see https://en.wikipedia.org/wiki/Materialized_view - also I felt it was important to distinguish from Drupal's use of the views as in the views module.

Ugh... nah I'm wrong. DBs implement both views and materialized views... old wires crossed in my head.

Gone for "database views" as the issue title here has... @oily thanks for questioning this!

@alexpott Thanks for considering my suggestion. The CR looks good!

Committed/pushed to 11.x, thanks!

I backported this to 11.2.x, then immediately changed my mind and reverted it again after re-reading the CR. This is an annoying bug (although for site admins rather than site visitors) but the important thing is to fix it in a branch of core, which was non-trivial, so not worth risking the theoretical disruption of an 11.2 or 10.x backport.

catch → closed merge request !12442

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