Port to 5.x

Patches would be gladly accepted.

I agree. This would be very useful in Drupal 5.

Thanks

+1 :)

yes, PLEASE ! (HEAD is now 6.x ! - 5.x is recommended installation)
at least one guru please tell where one has to pay attention and/or why/where creation of a .info file is not enough.

Subscribing.

it's actually not that hard to port from 4.7.x to 5.x. you just have to look at http://drupal.org/node/64279, go through the list, and see which of those apply to the code you're dealing with.

in the case of dba.module, the things needed are probably:

http://drupal.org/node/64279#info
http://drupal.org/node/64279#t-placeholders
http://drupal.org/node/64279#hook-settings
http://drupal.org/node/64279#administration
http://drupal.org/node/64279#drupal-get-form
http://drupal.org/node/64279#form-building
Change form_render() to drupal_render()
http://drupal.org/node/64279#format_plural
http://drupal.org/node/64279#drupal_add_js
http://drupal.org/node/64279#sentence_capitalization
http://drupal.org/node/64279#confirm_form

most of these require no special knowledge, just a little RTFM and some time with a text editor. often, search/replace is all you need. ;)

i really wanted this for a test site i'm working on. here's a start of the patch. still needs work, but a lot is done, so i wanted to share to avoid duplication of effort. status of the port:

all done:
http://drupal.org/node/64279#info
http://drupal.org/node/64279#t-placeholders
http://drupal.org/node/64279#hook-settings
Change form_render() to drupal_render()
http://drupal.org/node/64279#format_plural

not done at all:
http://drupal.org/node/64279#administration (we need to figure out what we want)
http://drupal.org/node/64279#confirm_form

i did a few, but not all of these:
http://drupal.org/node/64279#drupal-get-form
http://drupal.org/node/64279#form-building

mostly did this, but maybe missed a few:
http://drupal.org/node/64279#sentence_capitalization

didn't touch (but not necessarily critical):
http://drupal.org/node/64279#drupal_add_js

completed:
http://drupal.org/node/318 (Coding Style)
http://drupal.org/node/64279#info
http://drupal.org/node/64279#administration (now in admin/build/database)
http://drupal.org/node/64279#sentence_capitalization

incomplete:
http://drupal.org/node/64279#confirm_form (tried but failed)
http://drupal.org/node/64279#drupal-get-form
http://drupal.org/node/64279#form-building
http://drupal.org/node/64279#drupal_add_js

Since I need only a subset of dba functions for another module ATM, I can live with the current result. So do not hesitate to finalize this.

thanks for taking another stab at this!

however, it's not clear from your comment (and i haven't looked at the patch) -- did you expand on the patch i did or is this a new patch that doesn't include any of the work i already did? i'm confused because, for example, you mention (again) the info file, which my patch already handled...

also, i *just* briefly looked at the patch -- while i appreciate the attempt at coding style changes, it actually makes the 5.x port much harder to review and commit. i'd really prefer to handle those sorts of cosmetic changes in another issue and patch, so as not to make this one that much harder to read. but, what's done is done, so if it's not possible for you to re-roll the patch with only the 5.x-porting changes, so be it. however, please keep this in mind for future reference.

if you can start from my patch, add only the stuff you moved forward (basically the admin pages (i'm not sure /admin/build/database is the right place, but i guess that works), and whatever sentence capitalization in the UI i missed), and post only that, that'd be a big help for getting this committed and released sooner.

thanks,
-derek

My patch is based on yours. The module is already working with Drupal 5.x (as you can see here ;).

I've added "package = Development" in the info file, nothing else. All comments are now capitalized. The coding style took about 5 minutes actually, but unfortunately I did that first and without a VS - so I can't offer a roll-back.

However, only .../edit and .../remove actions are not working. I'm not yet very familiar with confirm_form() - based on my last test these paths were the only ones throwing some errors. Even Coder is almost quiet.

Besides from that I've already a patch that fixes BLOB and LONGBLOB escaping in SQL dumps. I don't know if anyone ever used that backup function - but it seems almost impossible that dba produced valid variable/cache exports ever. I'll file a separate patch for this later.

Here is the complete patch for a working 5.x version of dba.

This patch includes a fix for a bug that prevented a user to edit fields of the column 'description'. ('description' is reset by FAPI's confirm_form().)

Did I already mention that I hate confirm_form() ?

cool, thanks! i'll take a look ASAP.

so far, here's what i noticed:

1. if i select all tables and then use the "backup" button, the resulting backup.sql file contains the following:
   

   <script type="text/javascript">
         $(document).ready(function() {
           $("body").append("");
         });
       </script>
   

   needless to say, that causes errors when trying to restore from backup. ;)

2. there's no description of the "Database" menu item at admin/build/database, so the main admin page and the admin/build page look a little crappy.

i'll keep testing and see what else i find, and carefully review the patch. if i find anything else, i'll reply here. if someone wants to take a stab at fixing these 2 errors in the mean time, please post a new patch and set this back to "needs review".

thanks,
-derek

This looks like you suffer from this devel issue: http://drupal.org/node/131320

indeed, that solved the js problem. ;) thanks for the pointer. i just found some other bugs, but they're unrelated to the 5.x port so i'll open new issues for those. however, this still needs a description for the menu item. i'll keep chugging away and see what else i can find. thanks for all your work on this!

New patch including menu item description.

hehe, wrong patch. sorry, i really don't mean to be in a pushing match on this "needs work" status... ;)

Shot! :-D ...wrong directory... but that was another great patch ;)

Updated patch. Last patch did not contain the mentioned fix in #10.

Sorry for spamming. This is the last one.

I discovered another critical bug in dba's run_script() function. dba_run_script() used db_query() before, but db_query performs a variable substitution and tries to prefix tables. Because of that, all curly braces have been removed from all SQL statements, which leads to wrong serialized data, especially for arrays.

dba_run_script() now directly uses _db_query(), the helper function of db_query().

@dww/jeremy: If you agree, I'll commit this patch to HEAD and create a new DRUPAL-5 branch afterwards. That way, any new fixes can be reviewed more easily and we'll get more valuable feedback from Drupal 5.x users who do not want or don't know how to patch.

@dww @jeremy: any go-ahead for sun to create a DRUPAL-5 branch? That would really clear the roadblocks for progress on this.

sorry, yeah, we ran into an unexpected delay: DRUPAL-SA-2007-013. :( after testing the 5.x port, i realized there were a *ton* of XSS holes in this module, and further audits revealed other vulnerabilities. it took an over 700 line patch to fix all the problems (which is now committed to HEAD and DRUPAL-4-7). so, the latest patch for 5.x support no longer applies and needs to be re-rolled.

Sorry for the delay, I'm quite busy at the moment.

This looks quite promising. I just ran CVS update command on the last port for 5.x and fixed a few conflicts.

During testing I found only one bug that I don't know how to fix currently: After selecting multiple tables to check and submitting the form the selected tables are checked. However, after clicking on Check again I'm redirected to admin/build/database along with the message You must select the tables to check., i.e. the selected tables are no longer stored in the form/session. @dww: Could you have a look at this?

Another issue: $GLOBALS['form_values'] in dba_check_table_form_pre_render() is always empty. Don't know if that affects form_get_errors() there.

Performing actions on multiple tables seems to have nothing to do with this port. Multiple selected tables are also lost in the backup form in 4.7, as reported in http://drupal.org/node/138877.

Okay, so this is a little confusing because dba.module_4.patch looks like it applies to version 1.46 NOT version 1.48 (HEAD).
So I applied the patch to 1.46 and I was getting errors with the Query page.

The error came in db_query_page() where in the patched version it said $output = drupal_get_form('dba_query_form');
In the HEAD version it's fixed to $output = dba_query_form();

Sun can you clear up my confusion about the patch? I assume the HEAD version applies to Drupal 5, but it has no .info file...

Please?
This module is a bit big and unstable for me to dive into, but if we can start the 5.0 branch and release where it's at so far I can dig in and see what needs fixing.
As Sun has done already, this could be a useful base for a few other modules, not just demonstration sandbox but also potentially backup and site replication ... if this could provide more of a configurable API sorta set of functions.

... just a bump :-)

I'm sorry, because my last client is insolvent, I have to work on sponsored projects currently. Although I really would like to complete this port, I need to keep this on hold until I've completed some other projects - or - as long as there are no volunteers willing to contribute towards a fund - which I doubt since dba module is rather for development purposes.

Understood.
I go in and out of that phase also.
However, like I explained, I can't help do it myself while it's in the in-between phase it's at ...
Never mind.

Attempted patch on 1.46 and 1.48 (HEAD). Errors on both.

Please can we get a stable-ish version so a DRUPAL-5 branch can be created? At this point, I have no idea what to patch and or what to test. So I cannot help out ... even if I knew what I was doing.

This is a patch you can apply to latest dba.module ver. 4.7 with the same effect. Prior patches applied to 4.7.x-1.2 gave an error. This one shouldn't.

We need a patch that applies cleanly to HEAD, since that's where the D5 porting will be committed. This patch mostly applies there, but a few chunks still fail. Anyway, after such a patch lands, we can make a DRUPAL-5 branch, but until then, making the branch would just create additional work.

Also, .info files checked into CVS shouldn't define version anymore. http://drupal.org/node/152819

thanks,
-derek

subscribing to this

This patch applies to head version of dba. Query form is running fine. Script form would need some checking, although the upload form is looking fine. Also, I applied a patch to the dba_auto_backup function so it now works for demo.module, so yes, I recovered a demo site running demo.module and this dba patch. Info file also doesn't contain the $Id$ line.

Only one question. If there is a patch that makes dba working for 5.x, why isn't there any 5.x-dev release we can install and test? I think it mustn't be that difficult to set up a dev version. That way we could fill more reports. Please, do it soon. Demo module depends on this other module.

@robertgarrigos: thanks, but it's the version = "$Name$" line we want to remove from .info files, not the ; $Id$ part. ;) I could just fix this whenever I get a chance to do a final commit (assuming Jeremy isn't around to do it first).

@Farreres: there's no branch because the patch hasn't been committed yet, since there has yet to be a commit-worthy patch. We were close months ago, then I found the massive security problems and fixed them. After that, our previous efforts needed lots of re-doing, but I got way too busy with my day job, the D6 code freeze and other things, and "sun" has mostly disappeared, so this has been languishing in the queue, waiting for people to give it some much needed love. This is finally starting to happen again, so there's hope.

I'll try to review, test, and commit this latest patch sometime early next week, and make a 5.x-1.0-beta release...

oh, and robertgarrigos, i've already said i didn't like the fact this patch was changing a ton of code style stuff, too. i'd rather keep this just to D5 porting. so, i strongly object to including a change to dba_auto_backup() to better support demo.module going in at the same time. please file a separate issue about that, describing the changes you want to see, with a separate patch. if it's a pain to do the other patch until this one lands, so be it, but i don't want to commit your latest patch in here as-is.

thanks,
-derek

Right. No problem. Here it is. Drupal 5 ready, hope so, but not demo.module ready. Also .info file without version line but with $Id$ line.

This issue is driving me nuts. This patch sucks for many reasons (mostly due to sun's inability to heed my advice and requests early in this thread -- no offense to the folks who more recently tried to help). A bunch of the "code style" fixes (that have no place being here in the first place) are wrong and don't actually follow the code style. :( There are bug fixes that should be backported sprinkled in at various spots. There are new features or API changes still sprinkled in that don't belong here at all.

Part of me wanted to take my last clean D5 porting patch (#7 above), and finish *JUST* doing the D5 port on my own. That probably would have been better. But, I went through the latest patch, line by line, and started fixing the remaining problems that hold up this patch. 350 lines of patch to go. I'll just commit when I'm done.

Please let this be a lesson to anyone that porting patches should just port!!!. Wonder why it took months for this to get in? That's why. And, this is going to make it much harder to fix bugs in the future, since patches will be less likely to apply cleanly to both branches. :(

grrrr....

Committed a (highly modified) version of the last patch to HEAD:
http://drupal.org/cvs?commit=71245

5.x-1.x-dev snapshot now available:
http://drupal.org/node/94927

At the very least, checking tables is totally broken, but I didn't have time to fix all that right now. If someone wants to earn some dww-love-points, they'd a) file a separate issue, b) debug the problem, c) post a patch. ;)

If anyone finds anything else wrong in HEAD right now on D5, please file a separate issue!

Thank you, Derek, for seeing this through to completion! Your efforts and dedication are greatly appreciated. :)

I hope to be in a position myself to give the issue queue some love next month.

Yay!
Thanks for this.
I sympathise with both sides of the story here. :-/
I find it really hard to split my head in three when trying to help fix up a module. In the course of porting, it's hard to resist repairing code style issues where I see them, and usually the only reason I'd be opening up the code in the first place is to actually bugfix or add a feature. This inevitable leads to a three-in-one patch :(
However, even if I did get enough encouragement to go back and try to branch those bits into 3 separate patches ... how does one go about that?

Should random style fixes come first? (effectively repairing a release you are not even intending to, or even able to, run - boring)
Or should the port come first, ignoring style changes? (Which I'd find really hard as I often have to unfold the old code to the point I can understand what it used to do)

Absolutely no reflection on Derek here, I truly respect how tricky it can be to slide these things through, expecially with "Real Life" getting in the way.

Anyway. I see that this issue is off topic for here, so I'll shift it into its own discussion topic.

Thanks HEAPS for getting this to a branch release state.

I'll start poking it and see if I can plug in my planned site backup/deploy/synchronise tool.

FYI to all: I just spent ~2.5 hours methodically going through the differences between the DRUPAL-4-7 branch and HEAD, and did a bunch of further cleanup and unification:

1. repair a few things which snuck past me this morning, which we wanted to keep from DRUPAL-4-7, but which were lost by the monster patch, and finish repairing the code style in HEAD (http://drupal.org/cvs?commit=71273)
2. backport all the code style fixes to DRUPAL-4-7 (http://drupal.org/cvs?commit=71275)
3. backport a bug fix that i included in my earlier commit to HEAD which belongs in DRUPAL-4-7, too (http://drupal.org/cvs?commit=71277)
4. backport some code simplification i included in my earlier commit to HEAD which also belongs in DRUPAL-4-7 (http://drupal.org/cvs?commit=71278)

So, things should hopefully be much more sane going forward, patches should only conflict if they hit something that actually is different (menu paths, t() placeholders, etc). yay.

@dman: I replied to your forum post. Thanks for not hijacking this thread. ;)

Cheers,
-Derek