In trying to solve http://drupal.org/node/138877 I uncovered some quite nasty stuff. Basically, dba_get_active_tables() and everything that touches it is incredibly convoluted and fragile. :( Also, Jeremy, I know we've discussed before how crazy it is that we basically construct an array of tables, but pass it around everywhere as a comma-separate string, and are constantly imploding/exploding, str_replace'ing, etc, etc.

Feeling bold, I reorganized and re-wrote a bunch of this. However, I wanted to post it here as a separate issue so that it doesn't get any bigger than it already is...

Here are the important changes:

  • $tables is now always an array.
  • Majorly cleaned and simplified dba_get_active_tables(), and added a big phpdoc comment to explain what it's doing and what the args are.
  • Ripped out some crazy logic in a few menu callbacks and submit handlers that tries to recover from when you submit the overview page buttons without selecting any tables. Now, there's just a validate callback for the overview form itself to ensure we selected something. Much cleaner.
  • Tried to rename functions and variables that really handle multiple tables to use "tables" in their names, and ones that definitely are only dealing with a single table to just be called "table".

Disclaimer: this doesn't completely work. The confirm forms for the 3 operations that require confirmation (drop, empty, and backup) are broken. However, they're already broken in DRUPAL-4-7 (that's what http://drupal.org/node/138877 is about). So, you can expect those operations to fail when testing this patch. ;)

However, everything else works (at least as far as I can tell from my testing) and the code is much cleaner and simpler to work with. I'm convinced that once this lands, the patch to solve #138877 will be small and relatively simple.

Jeremy, mind if I commit this? Anyone else care to test, review, comment, etc?

Thanks,
-Derek

CommentFileSizeAuthor
#3 tables_array.patch15.93 KBsmk-ka
#2 dba_tables_array.HEAD_.patch.txt15.09 KBdww
dba_tables_array.47x.patch.txt15.31 KBdww

Comments

jeremy’s picture

jeremy commented

Anything that simplifies or cleans up the logic of the module is very welcome. As we've discussed before, it's a bit of a mess. So yes, go ahead and commit your patch once you're happy with it. If the cleanup breaks things, we'll fix that as we come to it. I've only had time to take a precursory glance, but based on that it looks good.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new dba_tables_array.HEAD_.patch.txt15.09 KB

cool, sounds good. however, i'm too tired right now to feel safe committing. ;) plus, a little more review/testing wouldn't hurt...

meanwhile, here's a patch for HEAD.

smk-ka’s picture

smk-ka commented
Version: 4.7.x-1.x-dev » 5.x-1.x-dev
StatusFileSize
new tables_array.patch15.93 KB

The patch for HEAD broke a lot of stuff, of which I've fixed:
- The local tasks for a single table have been fixed.
- The dba_check_table_form_pre_render() function was horrible broken.
- Tried to eleminate 4.7'ish stuff (accessing $_POST['op'] etc).

I know some fixes are not directly related to the tables array, but it's currently too hard for me to figure out which ones (f.e. the broken 'check' form probably is a separate issue).

Apart from the 'check' action (which required most work), I've only done smoke testing on the tables overview page (using multiple tables) and on single tables (but not emptying or dropping).
--
Stefan Kudwien
unleashed mind

dww’s picture

dww
we/he/they
commented
Version: 5.x-1.x-dev » 4.7.x-1.x-dev

I appreciate that you're taking a look here and trying to help. However, I think you misunderstand the point of this issue and patch. Many things are currently broken in HEAD, and this patch does not try to fix them all. It's just the basis for a cleaner codebase that will make it easier to fix everything in subsequent issues.

Also, I won't be committing anything here to 5.x-1.x-dev that isn't ready for 4.x-1.x-dev. And please don't hijack issues with D5 porting-related tasks that belong in their own issue (e.g. $_POST['op']). I haven't looked closely at your patch, but from your comments here, it seems like it's probably not going to speed up getting this into the code. I'd love to avoid another fiasco like the 5.x porting issue: http://drupal.org/node/106576.

A good way to figure out what's related to this and what isn't: try the functionality using the code currently in HEAD without the patch, and then try again with the patch. If it's broken in both places, it shouldn't be fixed here. ;) If something is *only* broken after applying this patch, then by all means, please fix it here.

Thanks,
-Derek

smk-ka’s picture

smk-ka commented

"4.7's dead, baby, 4.7's dead."

Since this clearly is a task ("reorganized and re-wrote a bunch of this") and not a bug, I surely won't start digging 4.7 up. Sorry, that ain't gonna happen.

If there are bugs, please issue separate bug reports, and these should be fixed even for 4.7. If nobody finds out about these bugs, then simply nobody's interested. I only took *your* patch and tried to make it work.

My 2 cents.

dww’s picture

dww
we/he/they
commented

I'm tired of getting in fights with people in issue queues, so I'm not going to continue down that road...

Let me just say a few things with a clear head:

1) DBA still doesn't really work in 5.x. 4.7.x is the only supported version, so that's what we're still supporting.

2) My patch was never intended to solve all the problems in 5.x. It was simply an attempt to make the dba code easier to maintain in the future (all branches), and in particular, to make it easier to solve http://drupal.org/node/138877. I'm pretty sure that once this patch goes in, solving that other issue will be on the order of 5 lines of code.

3) I already supplied patches for 4.7.x and 5.x. All this issue needed was someone to test that no functionality *got worse* as a result of applying these patches, since, IMHO, they certainly improve the clarity and quality of the codebase.

4) Since we're still supporting 4.7.x and trying to support 5.x, I'd prefer to keep the code bases in sync as much as reasonable so that future bug fixes can more easily be made in both branches. Hence my interest in doing this cleanup/reorg in both branches.

5) Separate bugs belong in separate issues. I couldn't agree more. That's why the $_POST['op'] stuff and t() placeholder changes you made in your patch don't belong here.

My $0.02.

So, if you want to help dba.module, please just do what this issue is asking for: review my patches, test them, and see if they make things any worse. If not, I'll commit them, and we can resume work getting the 5.x version working in other issues.

Thanks,
-Derek

dww’s picture

dww
we/he/they
commented

p.s. I just double checked and http://drupal.org/node/138877 is a critical bug in 4.7.x, too. Hence my interest in getting this in.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I tested this in 5.x, but I'm not sure what pieces should be working or not. The piece I care about and that worked before (the optimize function) worked as well after.

jeremy’s picture

jeremy commented
Status: Needs review » Reviewed & tested by the community

A quick read through this patch, I don't see any problems. Are we waiting to apply it for any special reason, or did we all just get too busy?

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

I was way too busy, and had gotten a little burned out on dba without help testing or reviewing this, so I wasn't just going to commit it on my own. But, since you're back at the queues, I just committed the original patch to DRUPAL-4-7 and #2 to HEAD. Progress! ;)

Now, I bet the fix for http://drupal.org/node/138877 will be a relatively small patch (thankfully). That should be our next task.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)