- Issue created by @neclimdul
- π³π±Netherlands daffie
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.
- π·πΊRussia Chi
The issue only applies to MySQL/MariaDB views. Right?
- π¬π§United Kingdom catch
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.
- πΊπΈUnited States neclimdul Houston, TX
I guess this just needs someone to write a patch?
- Status changed to Needs work
almost 2 years ago 8:16pm 4 August 2023 - πΊπΈUnited States jrb Raleigh-Durham Area, NC, USA
Here's a patch to get things started. This will:
- Add an optional parameter to findTables() to exclude SQL views from the query.
- Use that parameter in the mysql_requirements() call in mysql.install.
- last update
almost 2 years ago Custom Commands Failed - πΊπΈUnited States anoopjohn Washington D. C.
The patch works correctly on a 10.1.6 drupal site and stops showing warnings against views in the database.
- πΊπΈUnited States micahw156
Confirming this patch worked for me on 10.1.6 as well.
- π¦πΊAustralia apiddington
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.phpThis patch seems to work OK with v11.2: 3362726-9.exclude-views-from-table-list-11.2.patch
- πΊπΈUnited States nicxvan
Can you convert this to an MR?
Tests don't run against patches.
- First commit to issue fork.
- π¬π§United Kingdom oily Greater London
Test coverage might involve:
https://git.drupalcode.org/issue/drupal-3362726/-/blob/3362726-read-comm... - πΊπΈUnited States nicxvan
Did you accidentally remove the actual change? I only see the test stub.
- π¬π§United Kingdom oily Greater London
I havent applied any of the patches yet. It needs to be done.
- π¬π§United Kingdom oily Greater London
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?
- π¬π§United Kingdom oily Greater London
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..
- π¬π§United Kingdom oily Greater London
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.
- π¬π§United Kingdom oily Greater London
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.'
- πΊπΈUnited States nicxvan
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.
- π¬π§United Kingdom alexpott πͺπΊπ
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.
- π¬π§United Kingdom alexpott πͺπΊπ
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!
- π¬π§United Kingdom alexpott πͺπΊπ
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.
- π¬π§United Kingdom alexpott πͺπΊπ
My MR now also includes a fix that's limited to MySQL (and MySQLi) where the problem occurs.
- π¬π§United Kingdom alexpott πͺπΊπ
alexpott β changed the visibility of the branch 3362726-read-committed-requirement to hidden.
- π¬π§United Kingdom oily Greater London
@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?
- π¬π§United Kingdom alexpott πͺπΊπ
#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.
- π¬π§United Kingdom oily Greater London
@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.
- π¬π§United Kingdom oily Greater London
Test coverage in place. Test-only test fails as it should.
Can move to RTBTC?
- π¬π§United Kingdom oily Greater London
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?
- π¬π§United Kingdom alexpott πͺπΊπ
@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.
- π¬π§United Kingdom alexpott πͺπΊπ
Ugh... nah I'm wrong. DBs implement both views and materialized views... old wires crossed in my head.
- π¬π§United Kingdom alexpott πͺπΊπ
Gone for "database views" as the issue title here has... @oily thanks for questioning this!
- π¬π§United Kingdom oily Greater London
@alexpott Thanks for considering my suggestion. The CR looks good!