simpletest should not contain DB specific code [#327480]

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

#9	generalize_like_tables.patch	3.51 KB	chx

#7	generalize_like_tables.patch	3.67 KB	chx

#6	generalize_like_tables.patch	3.62 KB	chx

#4	generalize_like_tables.patch	3.61 KB	chx

#1	generalize_like_tables.patch	3.57 KB	chx

	generalize_like_tables.patch	2.78 KB	chx

Comment #1

chx CreditAttribution: chx commented 29 October 2008 at 05:13

File	Size
generalize_like_tables.patch	3.57 KB

Help if I roll a complete patch.

Log in or register to post comments

Comment #2

Crell CreditAttribution: Crell commented 29 October 2008 at 06:07

Status:

Needs review

» Needs work

This 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!

Log in or register to post comments

Comment #3

Damien Tournoud CreditAttribution: Damien Tournoud commented 29 October 2008 at 06:51

I would also argue that the "remove the tables that are in the schema" part should go away too.

Log in or register to post comments

Comment #4

chx CreditAttribution: chx commented 29 October 2008 at 23:09

Priority:	Normal	» Critical
Status:	Needs work	» Reviewed & tested by the community

File	Size
generalize_like_tables.patch	3.61 KB

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

Log in or register to post comments

Comment #5

Damien Tournoud CreditAttribution: Damien Tournoud commented 29 October 2008 at 23:21

Status:

Reviewed & tested by the community

» Needs work

Sorry 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).

Log in or register to post comments

Comment #6

chx CreditAttribution: chx commented 29 October 2008 at 23:27

Status:

Needs work

» Needs review

File	Size
generalize_like_tables.patch	3.62 KB

Fixed both.

Log in or register to post comments

Comment #7

chx CreditAttribution: chx commented 29 October 2008 at 23:36

File	Size
generalize_like_tables.patch	3.67 KB

Very nice tip from DamZ (fetchAllKeyed(0,0) ) made the simpletest code nicer and the result more useful and simpler.

Log in or register to post comments

Comment #8

Damien Tournoud CreditAttribution: Damien Tournoud commented 29 October 2008 at 23:37

Status:

Needs review

» Reviewed & tested by the community

Good enough for me.

Log in or register to post comments

Comment #9

chx CreditAttribution: chx commented 29 October 2008 at 23:54

File	Size
generalize_like_tables.patch	3.51 KB

Dropped $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.

Log in or register to post comments

Comment #10

mfer CreditAttribution: mfer commented 30 October 2008 at 00:25

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

Log in or register to post comments

Comment #11

Dries CreditAttribution: Dries commented 30 October 2008 at 08:56

Status:

Reviewed & tested by the community

» Needs work

The 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?

Log in or register to post comments

Comment #12

Crell CreditAttribution: Crell commented 30 October 2008 at 17:15

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

Log in or register to post comments

Comment #13

chx CreditAttribution: chx commented 30 October 2008 at 22:37

So people, what do we want to call the method that finds tables which begin with the name that is passed in?

Log in or register to post comments

Comment #14

chx CreditAttribution: chx commented 30 October 2008 at 23:08

Status:

Needs work

» Needs review

File	Size
find_tables.patch	3.76 KB

So I renamed and generalized more based on a hasty IRC discussion. It's fairly nice now I think.

Log in or register to post comments

Comment #15

sun

German

Karlsruhe

CreditAttribution: sun commented 30 October 2008 at 23:23

Status:

Needs review

» Needs work

Wrong indendation:

+/**
+  * Find all tables that are like the specified base table name.
+  *

Quote the example value?

+  *   An SQL expression, for example simpletest% . BEWARE: this is not

Double return:

+
+

Wrong indentation (different!):

+  /**
+   * Find all tables that are like the specified base table name.
+   *
+  * @param table_expression
+  *   An SQL expression, for example simpletest% . BEWARE: this is not
+  *   prefixed, the caller should take care of that.
+   * @return
+   *   Array, both the keys and the values are the matching tables.

Log in or register to post comments

Comment #16

chx CreditAttribution: chx commented 30 October 2008 at 23:38

Status:

Needs work

» Needs review

File	Size
find_tables.patch	3.78 KB

Are you channeling webchick there? You sound awfully like her.

Log in or register to post comments

Comment #17

Damien Tournoud CreditAttribution: Damien Tournoud commented 30 October 2008 at 23:43

Status:

Needs review

» Reviewed & tested by the community

That's even nicer than the previous one.

Log in or register to post comments

Comment #18

chx CreditAttribution: chx commented 31 October 2008 at 00:17

File	Size
find_tables.patch	4.8 KB

Pffft system tests failed. (empty arrays are casted to FALSE and nonempty are casted to TRUE by assertTrue/False.)

Log in or register to post comments

Comment #19

Damien Tournoud CreditAttribution: Damien Tournoud commented 31 October 2008 at 00:22

This patch is cursed :)

I just confirmed that the whole suite passes now. Keeping the status.

Log in or register to post comments

Comment #20

hswong3i CreditAttribution: hswong3i commented 31 October 2008 at 01:26

Sorry for get into this issue late. Maybe we want all tables retrieved and then process those?

Log in or register to post comments

Comment #21

chx CreditAttribution: chx commented 31 October 2008 at 02:11

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

Log in or register to post comments

Comment #22

hswong3i CreditAttribution: hswong3i commented 31 October 2008 at 02:42

Get it. Just subscript that issue and will move focus to there.

Log in or register to post comments

Comment #23

Dries CreditAttribution: Dries commented 31 October 2008 at 11:14

Status:

Reviewed & tested by the community

» Fixed

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

Log in or register to post comments

Comment #24

sun

German

Karlsruhe

CreditAttribution: sun commented 31 October 2008 at 12:03

Status:

Fixed

» Reviewed & tested by the community

File	Size
drupal.find_tables.patch	1.24 KB

meh! @chx: Go fix your editor!

Note to self: Don't go to sleep when chx drives people nuts for getting a patch in.

Log in or register to post comments

Comment #25

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 31 October 2008 at 15:17

Status:

Reviewed & tested by the community

» Needs work

Also, 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