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.
'Cos then how do you override it? This patch moves the simpletest_like_tables to db_get_like_tables, to the DB layer where it belongs. Untested but I need Crell's eyes on this ASAP as it blocks SQLite.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal.find_tables.patch | 1.94 KB | sun |
#24 | drupal.find_tables.patch | 1.24 KB | sun |
#18 | find_tables.patch | 4.8 KB | chx |
#16 | find_tables.patch | 3.78 KB | chx |
#14 | find_tables.patch | 3.76 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedHelp if I roll a complete patch.
Comment #2
Crell CreditAttribution: Crell commentedThis looks like a good approach for now. Schema introspection belongs in the schema system. We'll get around to rewriting Schema API later. :-)
So +1 on the approach, but -1 on the current code. It's still highly buggy just from visual inspection, Like, variables used without being defined. Bad chx!
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedI would also argue that the "remove the tables that are in the schema" part should go away too.
Comment #4
chx CreditAttribution: chx commentedAs Crell liked it and now works I dare to say it's RTBC. What Damien asks is a next patch. I bumped this to critical as it holds up SQLite.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedSorry chx,
- getLikeTables() is badly indented
- its PHPDoc is not consistent with its behavior (in the current implementation, tables that are in the schema are not returned).
Comment #6
chx CreditAttribution: chx commentedFixed both.
Comment #7
chx CreditAttribution: chx commentedVery nice tip from DamZ (fetchAllKeyed(0,0) ) made the simpletest code nicer and the result more useful and simpler.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedGood enough for me.
Comment #9
chx CreditAttribution: chx commentedDropped $target after a discussion -- it would nto be different for different targets. We tried to add different databases but then schema API as it is does not support yet. Let's not overdo it.
Comment #10
mfer CreditAttribution: mfer commentedDamien Tournoud said his RTBC still stands and I agree.
There is an unrelated error that came up while running the tests (on the next page load) which reads something like "Warning: call_user_func_array(): First argument is expected to be a valid callback, 'database_test_query_alter' was given in drupal_alter() (line 2896 of /Users/mfarina/workspace/drupal/includes/common.inc)." But, this comes up on a clean copy of head and isn't related to this patch.
All database tests pass after this patch.
Comment #11
Dries CreditAttribution: Dries commentedThe part that worries me about this patch, is that it might be permanent. As they say: "nothing is more permanent than a temporary solution".
I find the name of the function to be slightly odd. Also, "get_like_tables" exposes too much of the implementation. How about a more generic "search_tables" or somthing? Thoughts?
Comment #12
Crell CreditAttribution: Crell commentedPermanent or temporary, something that needs to be different on different databases belongs in the driver classes.
Using "like" in the method/function name makes sense if that's the API it accepts, vis, it behaves like a LIKE query. If not, then it should be something else. findTablesLike() works if that's the input. findTablesBeginningWith() is probably too verbose. Hm.
Comment #13
chx CreditAttribution: chx commentedSo people, what do we want to call the method that finds tables which begin with the name that is passed in?
Comment #14
chx CreditAttribution: chx commentedSo I renamed and generalized more based on a hasty IRC discussion. It's fairly nice now I think.
Comment #15
sunWrong indendation:
Quote the example value?
Double return:
Wrong indentation (different!):
Comment #16
chx CreditAttribution: chx commentedAre you channeling webchick there? You sound awfully like her.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat's even nicer than the previous one.
Comment #18
chx CreditAttribution: chx commentedPffft system tests failed. (empty arrays are casted to FALSE and nonempty are casted to TRUE by assertTrue/False.)
Comment #19
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis patch is cursed :)
I just confirmed that the whole suite passes now. Keeping the status.
Comment #20
hswong3i CreditAttribution: hswong3i commentedSorry for get into this issue late. Maybe we want all tables retrieved and then process those?
Comment #21
chx CreditAttribution: chx commentedGetting all tables is not what this issue is about. You want #301038: Add a cross-compatible database schema introspection API instead. The problem here is, I do not want to retrieve all and then have PHP pour over them.
Comment #22
hswong3i CreditAttribution: hswong3i commentedGet it. Just subscript that issue and will move focus to there.
Comment #23
Dries CreditAttribution: Dries commentedExcellent. Much better, and a lot closer to what I had in mind. Thanks for following through and not giving up on me. ;-)
Committed to CVS HEAD.
Comment #24
sunmeh! @chx: Go fix your editor!
Note to self: Don't go to sleep when chx drives people nuts for getting a patch in.
Comment #25
webchickAlso, the PHPDoc comments for db_find_tables() and findTables() needs to match; additionally, it should be param $table_expression, not param table_expression.
Nice job channeling me otherwise, though! I appreciate it. :D
Comment #26
sunNice catches. ;)
Comment #27
webchickThanks, committed! :)
Comment #28
c960657 CreditAttribution: c960657 commentedIt looks like this caused a regression in system.test: #328719: Module list functionality test fails
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.