Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This is step 1 for #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible.
Proposed resolution
Create the command/script.
Remaining tasks
User interface changes
API changes
Suggested commit message
Please ensure to add larowlan for his initial work on the script in #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible.
Comment | File | Size | Author |
---|---|---|---|
#69 | hook-update-n-test-script-2497323-69.patch | 26.2 KB | jhedstrom |
Comments
Comment #1
jhedstromComment #2
jhedstromHere's a fairly rough start that combines the starting script from #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible with a lot of the logic in
migrate-db.sh
. The console application could probably handle a lot of the bootstrapping that's still happening in the script itself.Unassigning as I won't have time until next week to pick this up again.
Comment #3
jhedstromNote, the console ansi coloring breaks php syntax, so to test generating, use
./dump-database-d8.php --no-ansi > my-test-script.php
Comment #4
jhedstromComment #5
jhedstromAdding some remaining tasks.
Comment #6
dawehnerCool tricky, so we have commands which can be reused, but we still have a working script.
This is part of a different issue, isn't it ;)
Comment #7
jhedstromI'm going to write some tests.
Indeed, I forgot to rebase the branch :)
Comment #8
webchickSince this is blocking a critical, escalating to critical.
Comment #9
jhedstromThis patch:
Comment #11
jhedstromUpdating issue summary. The tests will load the necessary scripts.
Comment #12
webchickTestbot seems to have failed a bunch of patches this afternoon but this one:
...seems like it could be legit.
Comment #13
dawehnerIts great to see good readable code here!!
Let's typehint the ModuleHandlerInterface
Do you really need the escaping of the $ here? At least
Here is a general question, no idea about a proper answer :)
\Symfony\Bundle\FrameworkBundle\Console\Application
gets the entire kernel injected, I guess this is fine, as its a much more generic code, but should our DbDump application also get the kernel or does the rule of "the less dependencies the better" apply here?Comment #14
jhedstromre: #12
The fail was legit. The
{}
were not making it through to the query, so the table was not properly prefixed. Also, I had not enabled the config module :)re: #13
I was previously using heredoc, switching to nowdoc so we don't have to escape the
$
.I originally limited the dependencies with the plan of using PHPUnit for the tests. Since we're using KernelTestBase, we could inject the kernel...not sure if it buys us anything.
Comment #15
jhedstromOops, missed an instance of ModuleHandler.
Comment #16
jhedstromComment #17
jhedstromThis disables the ansi output unless explicitly set. So now a script can be generated without needing to remember to set
--no-ansi
:Comment #18
jhedstromThis adds tests for actually loading the generated script. Writing these caught several errors in the script from previous patches (tests FTW).
I've also tentatively removed the trailing
.php
from the script, which is more in keeping with modern scripts (the php bit is in the header of the script). It also avoids having to call the scriptdump-database-d8.php
from within the command classes.Comment #19
jhedstromUpdating the issue summary to reflect current status of the script.
Comment #20
dawehnerOH that sounds quite scary for a test ... can't we maybe add a feature to export them with a different db prefix? I could imagine that this could be useful for actual upgrade path testing as well?
Well, this makes it more complex to look at the file with your editor, right? I'd just go with .php and be fine, honestly.
+1 for just adding what is actually needed.
Comment #21
jhedstromThis drops the test tables that are prefixed (simpletestNNNNconfig, simpletestNNNNsessions, etc).
Comment #22
jhedstromEven if we add the
.php
back to the script name, should we leave this as-is, or change the actual command name internally todump-database-d8.php
? The only place this surfaces is if a user types./dump-database-d8 --help
:Comment #23
cilefen CreditAttribution: cilefen commentedDrupal/Core/Command/DbDumpCommand and Drupal/Core/Command/DbDump are easy to confuse. Could there be another way to name those?
Comment #24
dawehnerYeah, what about using DbDumpApplication ?
Comment #25
jhedstromThis patch:
dump-database-d8.php
DbDump
class toDbDumpApplication
Comment #26
dawehnerWe have tests, do we need something more?
Here is just some continues review, sorry for the nitpicking!
I hate our super strict docs requirements which doesn't really add any value, but meh we need some one line docs here.
What about describing here what will be part of the dump: tables + modules ...
Let's add --ansi to the ::configure() description so its part of the help text.
Does that mean sqlite / pgsql tests will fail then?
I'm curious whether we also need enabled themes here?
Comment #27
jhedstromI've removed the remaining tasks section since they were all completed.
This patch should address #26 1 and 2.
It's already part of the help text as the option is provided by the default command class. (See output in #22).
No, it just means a developer running on either of those couldn't generate a new script. The generated script should work on any backend.
The enabled theme would be available in the exported config, and since themes don't create any tables, this is probably not needed?
Comment #28
dawehnerOh, got it. Sorry
Well, so we can't test the command itself, as the test for the command itself would try to generate a script, which does not work on other databases?
Oh wait, then I probably don't get the point of the modules ... Ah I see, its pure documentation purposes.
Comment #29
jhedstromOh, hmm. That is correct, these tests will fail on non-MySQL.
Comment #32
dawehnerI can't speak for things in general, but I would be fine for such an edge case "feature" to be opt in for mysql
Comment #33
jhedstromThis patch updates the tests to skip if not running on MySQL. I updated the documentation to indicate why this script is currently MySQL only.
Comment #34
jhedstromI discovered an issue with the exported script while working over in #2498625: Write tests that ensure hook_update_N is properly run.
Some mappings aren't correct. For instance, the
cache_discovery
table:Note the data field in the original table is
longblob
, while the table created from this script isblob
.Comment #35
jhedstromThis patch adds the 'size' parameter back to the schema where applicable. I also added tests to verify the generated schemas after loading the tables again.
Comment #36
dawehnerI'd recommend to check
Database::getConnection()->getDatabaseType()
Its the kind of code we do in other places as well .Do you mind opening up a follow up to make the script/test database agnostic?
Do you mind also opening up a follow up to provide this kind of functionality: From a given table, extract the schema. Its certainly not required as part of this issue, but would be nice in general
Comment #37
dawehnerWe talked about this issue on a European critical meeting.
a) the script should be renamed to ..._mysql to make it clear b) throw an exception and render some helpful message if the wrong database type is in use
There has been an issue already to have an API in core to extract a table schema from an actual table, but its quite an effort, so it should not block the upgrade path,
what we care about at the end.
Comment #38
amateescu CreditAttribution: amateescu as a volunteer commentedI agree with @dawehner that we need a followup to add some introspection capabilities to the db schema API, I would even say that it should be required in order to close #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible. Otherwise, the patch looks like an awesome start for a generic "db dump" command!
Comment #39
amateescu CreditAttribution: amateescu as a volunteer commentedCross-posted with #37; I'm also ok with not blocking the critical on the schema introspection issue and +1 to a) and b) :)
Comment #40
dawehnerI know that catch had an issue at some point, I guess its #301038: Add a cross-compatible database schema introspection API
@amateescu
I think its not required because the actual dump will be usable in every database, so it doesn't block you from testing the update path, you just need to use mysql in order to generate the dump, which is a fine limitation.
Comment #41
jhedstromThis adds the mysql-specific naming, and throws an exception if run from a non-mysql backend. I also added a link to #301038: Add a cross-compatible database schema introspection API to the docblock.
Comment #42
jhedstromPrevious patch had an unused SafeMarkup.
Comment #43
dawehnerI think this is a good step forward in order to resolve the actual important critical part of having an upgrade path! This would encourage way more people to jump on Drupal 8.
Comment #44
catchMy schema testing issue was #1119094: Add a test to verify the schema matches after a database update.
For that issue I had started to review https://www.drupal.org/project/schema which has the functionality we need, but at the time there was no Drupal 7 version and I stopped long before starting on a patch - looked like too much work. There is a Drupal 7 version now though.
Comment #45
alexpottI guess that makes sense. What happens if we want to test something that changes watchdog or session table structure?
Watchdog type before dumping:
Watchdog table after dumping:
Some fixes I'd make: remove an used use statement and fix the indentation in the script produced.
Comment #46
alexpottAlso note cache cids...
before dumping...
After reloading the dump.
Comment #47
alexpottAlso cache tables are missing their key...
KEY `expire` (`expire`)
and comment...
Yep it seems all non primary keys are missing.
Comment #48
larowlanFairly vacuous code review, manual testing later
%s/included/including
extreme nitpick™: %s/this/This
swoon
Comment #49
larowlanAnd something less vacuous
So it seems like we're going to a lot of effort to reverse generate a schema definition in Drupal's agnostic hook_schema format from an existing table. As pointed out by @alexpott, it is missing a few things like keys other than primary. Given we've already locked this to a MySQL specific script, I don't think there is any benefit in reverse-generating the schema in an agnostic hook_schema format. I think we'd be better off using MySQL's SHOW CREATE TABLE syntax in the dumper, storing that output as a series of SQL statments which can be executed directly in the loader.
It will mean a lot less code.
Thoughts?
Comment #50
jhedstromThe D7 method for this sort of thing allowed for multiple db dump files to be loaded, so that pattern could be used to populate these tables as needed, rather than rely on them being properly populated when the base db dump script is created.
I'll try to address the remaining feedback in a patch later today.
Comment #51
jhedstromThis would make the actual upgrade tests dependent on MySQL, which is currently not the case with this patch--only testing the script itself is dependent on MySQL, while the output can be used on all supported backends.
Comment #52
jhedstromChecking for indexes and collation is going to add quite a bit of code to the
DbDumpCommand
class that would need to be replicated into theDbDumpTest
class if we want tests.Would it be out of scope for this issue, and 8.0.x, to add a few new methods (
getTableSchema()
andgetIndexes()
) to themysql/Schema
class with the same@todo
messages currently in this patch?Comment #53
jhedstromHere's a patch that catches coalition info, and indexes. I didn't add tests for the coalition since setting those properties in the dump change the schema already being tested (eg, type = 'varchar_ascii'), so they are already tested in a way. I added tests for the indexes.
I have not refactored as suggested in #52, since I have no idea if we can do that refactoring at this time.
This should also address the nits from above.
Comment #54
dawehner@effulgentsia and @dawehner talked about this issue and we wondered why we can't call out to
mysqldump
to generate the dump?It would be fine to require the usage of that script IMHO, but it would make things way more safe, as all the usecases would be used ...
Comment #55
dawehner#54 is kinda wrong in the assumption that
mysqldump
is database independent. http://stackoverflow.com/questions/5417386/import-mysql-dump-to-postgres... for example shows that pgsql has issues with it.Given that the approach is probably much better, because it actually allows us to do cross database types update testing
Comment #56
dawehnerWhat about documenting why we can't use mysqldump?
What about trying to checkout watchdog as well?
Comment #57
jhedstromI added a note about why dump utilities cannot create a backend-agnostic sql file.
Comment #58
dawehnerDid some manual testing
Note: I think its an actual problem of this patch ... see below
Observations
vs
- KEY `menu_link_content_field__link__uri` (`link__uri`(30))
Comment #59
jhedstromThis should address ordering of all indexes (including primary keys), and it adds support for unique keys. It also adds support for index length. I added a few more modules and entity schemas to the test to ensure this.
Comment #60
dawehnerThank you!
Just in general, I think its totally fine to have this in one line
Comment #63
jhedstromPHPStorm got a little overzealous about code formatting and I didn't notice before I posted the patch :(
Comment #64
dawehnerYeah its not a problem IMHO.
Seriously testbot you are drunken
Comment #65
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC + 1
Comment #66
alexpottThere are still some unexpected differences.
Before
After
So this excludes tables created by the Simpletest module as well. I guess this is an edge case. And an upgrade path for Simpletest is a bit ridiculous. But it does mean that simpletest would be broken if this script was used on a real site.
Before
After
The uri column is not the same for some reason.
Comment #67
webchickSince this is a sub-issue of #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible, which is a D8 upgrade path blocker, tagging as such.
Keep up the fantastic work! :D
Comment #68
webchickMore metadata adjustment. Almost done, I swear...
Comment #69
jhedstromThis should address the 2 schema issues from #66. I left the simpletest logic in place for now.
Before
After
Before
After
Comment #70
jhedstromI also added semaphore and file_managed to the tests (which in turn required the user module and schema), so there is more test coverage now.
Comment #71
dawehnerThe feedback from alex got adressed.
There might be more edge cases, I mean SQL can do alot of stuff, but at least standard install is now safe.
Comment #72
alexpottSo do we want the script to executable?
Comment #73
jhedstromCurrent state of the scripts directory is mixed in terms of being executable or not.
Comment #74
jhedstromThat being said, I don't know that it really matters.
Comment #75
alexpottYep I saw the mish-mash - lets leave it like it is.
Committed 0b4add2 and pushed to 8.0.x. Thanks!
Comment #77
jhedstromCreated #2504167: Consolidate mysql-specific schema functions into \Drupal\Core\Database\Driver\mysql\Schema as a follow-up to my thoughts in #52. No idea if that is feasible in 8.0.x, or even in the 8.x.x cycle at all.