Don't know why, but with the latest versions (1.69+ at least) when you try and delete a column it doesn't do the _submit function even though the form id is clearly defined in confirm_form... This is puzzling and I'm going to need to get to the bottom of it before I can release any new features as its obviously a critical feature.

If you need it functional, 1.65 appears to be working.

Pobster

CommentFileSizeAuthor
#5 tablemanager-column-delete-bug-hack.patch1.65 KBmcurry
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pobster’s picture

pobster CreditAttribution: pobster commented

Hmmmm now this is interesting... It seems that 1.65 DOESN'T work either... This now leads me to conclude that the security patch affecting IDs has changed something somehow as I *know* that release worked beforehand...

...I'll look into it some more.

Pobster

davemybes’s picture

davemybes CreditAttribution: davemybes commented

Anything new to report on this matter? I'm trying to delete a column (with no data) and it won't do it. I will make a new table in the meantime, and if I have some time, I might take a look at the code.

pobster’s picture

pobster CreditAttribution: pobster commented

To be honest I gave up on trying to fix this as the current plan of action is to finish off the new 5.0 version which works using javascript instead of messy (and slow) php functions. Once I'm finished with 5.0 I intend on backporting it to 4.7 so I can update the pair of them together when necessary (there will be a facility to convert the db tables automatically using /update.php).

...a HUGE spanner in the works though is that I've lost my dev PC with months of work on it (real-life work and Drupal work together) I assume to a power surge as motherboard (probably processor too although I've not checked yet) and HDD are both toast :o( Yes, its a bit of a setback... :o(

Pobster

davemybes’s picture

davemybes CreditAttribution: davemybes commented
Status: Active » Postponed

No worries on fixing this, its simple enough to start again if the table isn't too big. Bummer about your computer - it happens to everyone eventually. Mine died just before Christmas, but luckily my harddrive survived.

I look forward to the 5.0 version.

(setting to postponed)

mcurry’s picture

mcurry CreditAttribution: mcurry commented
Status: Postponed » Needs review
FileSize
tablemanager-column-delete-bug-hack.patch1.65 KB

I'm reopening this, because I really need this to work - I can't afford to delete and recreate tables...

I don't know why, but the forms management system seems to route the confirmation back through tablemanager_tableedit(...) - rather than tablemanager_columndelete_submit(...)

I've traced thru the code, and found that the _tableedit() function is called after the user confirms the column deletion by pressing the "delete" button in the confirmation form. I don't know enough about the forms api, but it appears that the form's action field is causing the confirmation action to be routed thru the _tableedit() function rather than the columndelete_submit() function. I've compared various confirmation forms, and it appears that the action attribute is the immediate cause of the problem (again, I don't know enough about the forms api, but I do know that the current code is routing the column deletion form submit thru the tableedit() function, so I know something is fundamentally wrong with the form code for column deletion.

Column delete confirmation form HTML: (notice that the action and id don't match)


<form action="/admin/tablemanager/tableedit/1"  method="post" id="tablemanager_columndelete" class="confirmation">
<div><input type="hidden" name="edit[row]" id="edit-row" value="1"  />
<input type="hidden" name="edit[col]" id="edit-col" value="1"  />
This action cannot be undone and will destroy ALL table data.<input type="hidden" name="edit[confirm]" id="edit-confirm" value="1"  />
<div class="container-inline"><input type="submit" name="op" value="Delete"  class="form-submit" />
<a href="/admin/tablemanager">Cancel</a></div><input type="hidden" name="edit[form_id]" id="edit-tablemanager-columndelete" value="tablemanager_columndelete"  />

Delete table confirmation form HTML: (Notice that the action and ids match...)

<form action="/admin/tablemanager/tabledelete/1"  method="post" id="tablemanager_tabledelete" class="confirmation">
<div><input type="hidden" name="edit[id]" id="edit-id" value="1"  />
This action cannot be undone and will destroy ALL table data.<input type="hidden" name="edit[confirm]" id="edit-confirm" value="1"  />
<div class="container-inline"><input type="submit" name="op" value="Delete"  class="form-submit" />
<a href="/admin/tablemanager">Cancel</a></div><input type="hidden" name="edit[form_id]" id="edit-tablemanager-tabledelete" value="tablemanager_tabledelete"  />

</div></form>

Anyway, as a short-term fix, if we intercept the the $op="Delete" operation early in the _tableedit(...) function, and then forward to _columndelete_submit(...), the column is deleted as desired. Crude, but I don't have time to figure out a more elegant fix - because I don't know if this is due to a structural problem in the way tablemanager is trying to use the forms api, or if it's due to a simple error in the use of the forms api.

function tablemanager_tableedit($edit) {
  // check here to see if user has confirmed a delete
  $op = $_POST['op'];
  if (strcmp('Delete', $op) == 0) {
    $form['row']=$_POST['edit']['row'];
    return tablemanager_columndelete_submit(null, $form);
  }
...

Here's a patch against the DRUPAL-4-7 branch - if this quick (hack) fix is acceptable while we wait for the next release of this module -- and I'd love to hear from someone who knows more about the inner workings of the forms subsystem - what's wrong with the column deletion confirmation form code in tablemanager_tableedit() ? What is the correct technique to do this?

cvs -z9 diff -u -wb -F^function -- tablemanager.module (in directory C:\devtools\IRails\www\sites\default\modules\tablemanager\)
Index: tablemanager.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/tablemanager/tablemanager.module,v
retrieving revision 1.72.2.1
diff -u -w -b -F^function -r1.72.2.1 tablemanager.module
--- tablemanager.module	5 Dec 2006 13:30:45 -0000	1.72.2.1
+++ tablemanager.module	4 Feb 2007 06:43:17 -0000
@@ -385,6 +385,7 @@ function tablemanager_tabledelete($edit)
     t('Delete'),
     t('Cancel')
   );
+
   $output .= tablemanager_display($edit);
   return $output;
 } // tablemanager_tabledelete
@@ -1593,6 +1594,12 @@ function tablemanager_tables() {
  * Edits table header.
  */
 function tablemanager_tableedit($edit) {
+  // check here to see if user has confirmed a delete
+  $op = $_POST['op'];
+  if (strcmp('Delete', $op) == 0) {
+    $form['row']=$_POST['edit']['row'];
+    return tablemanager_columndelete_submit(null, $form);
+  }
   if (!is_numeric($edit)) {
     drupal_access_denied();
     return;
@@ -1610,10 +1617,10 @@ function tablemanager_tableedit($edit) {
     drupal_access_denied();
     return;
   }
-  $op = $_POST['op'];
   $col = substr($op, 16);
   switch (TRUE) {
     case is_numeric($col):
+    // this section is just bogus
       $form['row'] = array(
         '#type' => 'hidden',
         '#value' => check_plain($edit),
@@ -1845,6 +1852,7 @@ function tablemanager_tableedit_validate
   return;
 } // tablemanager_tableedit_validate
 
+
 /**
  * Submit function for tablemanager_columndelete_submit
  */

***** CVS exited normally with code 1 *****
davemybes’s picture

davemybes CreditAttribution: davemybes commented

Excellent work. Your patch works as expected. I have looked through the code to see if I can find the error, but my knowledge is just not there yet.

mcurry’s picture

mcurry CreditAttribution: mcurry commented

Yeah, I don't know how it's supposed to work, so I can't even begin to understand the nature of the problem. I spent about four hours poking around (I don't have a proper PHP debugger installed, so it was trial and error).

All I know is that the current implementation doesn't work as intended... and I don't know if it *ever* worked - does anyone know if there is a version in CVS that did work? I'd like to compare diffs..

-Mike | http://exodusdev.com

pobster’s picture

pobster CreditAttribution: pobster commented

Haven't tried your patch yet as no dev PC to try it on (all I've got is a live server) but anyways, I can confirm that it was a change to Drupal core that broke tablemanager, not the code itself so technically the current release should work on an older version of Drupal 4.7 (not that I've tested that theory!) It's only the link to the function which is broken not the function itself and that's the reason why I've never bothered to fix it as the restructuring of that entire area of code is going to be more complicated than just writing it a new way for the 5.0 version (I'm going to backport it as it works in a much better way).

Pobster

mcurry’s picture

mcurry CreditAttribution: mcurry commented

Ah, I see. Can you identify the Drupal core change that caused the breakage? Any hints are greatly appreciated, as I will be using Drupal 4.7 for quite some time, and I'd like to at least understand what went wrong here.

You mention a change to drupal core, do you know what version of Drupal 4.7 is known to allow this the column delete function to work, and what the first drupal 4.7.x release was the first to break it?

In any case, thanks for providing this highly useful module.

Mike | http://exodusdev.com

pobster’s picture

pobster CreditAttribution: pobster commented

Difficult to say now (as it was a while ago...) but I reckon looking at this: http://drupal.org/files/sa-2006-025/advisory.txt That it's probably 4.7.3 - I'm pretty sure I remember that it was the stuff about form ids/ tokens which broke it. It really did work absolutely fine at one point I promise!!! I seem to recall it being down to that you can now only specify one form to call from a button whereas it used to be however many you liked. Hence how comes only the first form gets called now... The completely easy fix is to just redo the edit page to have text links instead of form buttons, but like I said before I'm concentrating on finishing the 5.0 version and then backporting it to 4.7 so it'd be a waste of time for me really... Mind you, as I've mentioned on other posts my dev PC is fried and I'm waiting for my household insurance to pay out for a new one at the moment so everything is in limbo... The 5.0 version will work 'the Drupal way' as well, so all that messy code will go from the 4.7 version! Yay!

Pobster

davemybes’s picture

davemybes CreditAttribution: davemybes commented

Thanks for the info, pobster. So for now, it looks like inactivist's patch is going to have to stay until you get the backport done. I'm glad to hear that you're working on a 5.0 version too. After staring at the code for an hour or two last night, trying to figure out where the code got broken, I have so much more respect for you guys who code these things. I've written a few modules for specific situations, but nothing this complex - I think my brain would explode if I tried :)

Pobster, I'll let you change the status to fixed so that other people will know there is at least a (temporary) working patch available for the issue.

mcurry’s picture

mcurry CreditAttribution: mcurry commented

Might be better to set it back to 'postponed' - if you set it to 'fixed' , drupal.org bug tracking will auto-close the issue after a time of inactivity, and then people won't be able to find this unless they tweak issue filters...

In any case, I agree with incrn8. Let's mark this as non-active!

stevryn’s picture

stevryn CreditAttribution: stevryn commented

This problem occurs in 5.1 as well, so whatever fix there is for 4.7...will have to be applied to 5.1 as well. Pobster do you want a report filed under 5.1 as well?

pobster’s picture

pobster CreditAttribution: pobster commented

Nah - if you read the release notes for the new releases you'll see that they're only temporary releases anyway just to get people up and running with Drupal 5.0. These releases are not the new Tablemanager version just a stopgap.

Thanks

Pobster

stinky’s picture

stinky CreditAttribution: stinky commented

Any updates on getting this to work? I'm using Tablemanager 5.x-1.3 and Drupal 5.6 and can't delete columns.

shijmus’s picture

shijmus CreditAttribution: shijmus commented

I can not delete column too,

pobster’s picture

pobster CreditAttribution: pobster commented

Okay so after these last few comments I feel it's necessary to give some explanations to let you all know what's going on;

Currently I'm getting up at 6:45am and working from 7:15am till 9pm EVERY week day. I'm a futures trader, trading both Short Sterling and Euribor futures (hence the late finish) although I'm mainly in the Sterling (hence I don't start work at 3am like everybody else for the Euribor...) So yes, I do work from home staring bleary eyed at a PC screen but more often than not I'm so busy what with the current financial climate that I don't get time to even eat during the day, let alone do any unpaid programming. This literally does mean that I'm on my own here working on a PC solidly for nigh on 14 hours a day (I'm sure it's probably illegal?) with only my dog for company. Now ask yourself if you spent that much time in front of a screen Monday to Friday, "would you feel like spending any of your own time programming for free over the weekend?"...

Now you're all probably thinking, hmmm futures trader - I'm not sure what that is, but it sure sounds like you make a lot of money... Well no... I don't... I'm self employed and I trade my own money, meaning that if I lose any money it's literally gone instantly from my own bank account - and if I don't work - I don't earn anything. My trading costs are enormous, even if I do no business at all I still get charged monthly 'rental' of my trading software (which I assume was written by trained monkeys as the support is non-existent, I'm sure I could do a better job myself) this cost is £550 a month ($1100) roughly 30% of personal average monthly income for my local area (and nearly as much as most peoples monthly mortgage payments @ £590 http://www.newsroom.barclays.co.uk/Content/Detail.asp?ReleaseID=1047&New...) - anyways lets take an example day, my last full trading day, the 11th of January;

I made a total of £993 ($1986-ish) across the two markets but it cost me £1292.24 to trade on that ONE day ($2584.48) so I made a loss of £300 or so ($600). Does anyone here know how it feels to work for 14 hours straight, struggle to make nearly a thousand pounds and still LOSE money? Probably not... Well I can tell you, it's *more* than just disheartening... Anyway, that's why sometimes I can't face sitting in front of the computer at weekends... Weekends are currently a relief that I don't have to sit here any more... A rest for my eyes and brain...

Tablemanager v2 was fairly finished about six months ago - indeed I currently run it on one of my live sites, but it's heavily in dev still. Nowhere near being an alpha release let alone a beta. It works better and in a more simple way, it's more functional and uses a more sensible approach to storing the data contained within the tables. But as a result it's more complicated to code and this means that if I'm going to spend any time coding anything which I'm not being paid to code then I'm going to work on the new version and not the old one... Especially as someone has already posted a perfectly good hacky patch in this issue thread to solve the column delete redirection. I'm not bothered about fixing it, it's only the link to the function which a change to Drupal core broke - you can call the function manually or apply the patch, it's not rocket science... If it was a security hole I'd have plugged it already - I am conscientious after all - but a broken link? Nah, whatever... I've simply no time for something that unimportant, I'm too busy trying desperately to make enough money to pay my rent. The only way to make big money in this game is to have money already - and hmmm that's certainly not me...

I'm not after any pity here, I'm sure my life is better than the majority of people. Although my choice of job is hard graft and stressful, I've been involved in it in one way or another since I left school some 16 years ago and so, I'm experienced at it and hence, fairly good at it... I'm just after a little understanding that I'm not constantly 'on tap' to solve problems/ issues/ answer support questions. All work I have on here is *free* and freely downloadable, *this is NOT my job* nor does it provide me with any source of income. Please remember that when you're asking questions, especially questions which have already been answered. Even newbs can use the search function and rEaD threads, don't know how to apply a patch? Type "patch" into the search box - the first three items returned are; "Creating Patches", "Patches" (*about patches) and "Applying Patches"... All are book pages, meaning they're pages from the manual... Repeating a question on a thread which already has a working patch is just laziness... YES the patch is hacky, but to be honest the reason the change to Drupal core broke it is that the original code was hacky to begin with.

...So this version of Tablemanager will NEVER get fixed, what will happen is when I have a couple of days to myself I'll finish off version 2 and then backport it to both 4.7 and 5.x, don't hold your breath though I've not worked on Tablemanager in months... I've had a few freelance paid projects to do which obviously took preference and recently I'm just too busy working.

Pobster

mcurry’s picture

mcurry CreditAttribution: mcurry commented

@pobster: Understood. I've been in a similar situation (though nowhere near as intense) with some of my modules, and yet the change requests keep coming in, with nary an offer to help (financially or otherwise.)

Have I annoyed you? If so, I apologize. Never my intention. :D

Let me know how I can help. I can help test and debug modules and propose fixes.

pobster’s picture

pobster CreditAttribution: pobster commented

Nooooo no-ones annoyed me! The above wasn't aimed at anyone in particular. It was more of a plea to get people to just think a bit before posting. I'd say around 90% of the posts I answer can be solved simply by using google/ Drupals search, in fact that's generally how I answer them! Especially at the moment, my time is pretty precious and I don't really want to be answering easily answered questions when I should be working! Indeed at this precise moment in time I'm down £300 ($600) at work and so I really should be getting back to it!

Pobster

proactive1130’s picture

proactive1130 CreditAttribution: proactive1130 commented
Version: 4.7.x-1.x-dev » 5.x-1.4

I'm on the latest version and I can't delete columns either!
Does the patch work for this version also? If so, how do I apply it?

Thanks

pobster’s picture

pobster CreditAttribution: pobster commented

Well luckily for you, the current 5.x branch is just a simple bastardised conversion of the old 4.7 version so the lines you'd change are pretty much the same ones as in the patch (they won't be in the same place as the 4.7 version though so you can't simply apply the old patch - you'll need to seek out the corresponding lines).

The current 5.x version was never meant to exist... In fact Tablemanager V2 *was* meant to be the new 5.x version but then Drupal 6.x came along and I thought as I may as well just develop it for that...

Pobster

pobster’s picture

pobster CreditAttribution: pobster as a volunteer and at ArcadeGeek LTD for Rackspace commented
Issue summary: View changes
Status: Needs review » Closed (outdated)

Closing as D5.x is now unsupported.