Issues with cancel button form handling

I can confirm this in both Garland and Marvin.

Well you just need to add some CSS to let it look like a button, its simply not themed and its not an input field.

Maybe i should add some nice CSS for garland, but thats rather i feature request.

The problem is that it's a link - not a button. The form widget type should be button and not markup, so it conforms. I'm putting a patch together to take change this. I also see some problems with the IF statements detecting a node add vs an edit. And I noticed that the module code does not adhere to Drupal coding standards. You should read over the standards page http://drupal.org/coding-standards#indenting.

A quick example:

Wrong:

if( $form['#id'] == 'comment-form' ) {

Right:

if ($form['#id'] == 'comment-form') {

Also, not sure why user-profile-form is in _content_lock_add_cancelbutton() as it looks like it's never called when viewing the user profile form..

I also see that the comment-form #value is node/nid/canceledit - why wouldn't a comment ID be passed at all? What does adding a comment to a node have to do with editing or locking?

I'm confused now..

I'm not seeing anything in hook_menu to handle unlocking actual users or comments, etc...

I'm going to remove that unneeded stuff in _content_lock_add_cancelbutton as well as the menu callback and let the forms handle everything much more cleanly..

It also looks like there are no permission restrictions and anyone can visit node/%nid/canceledit to release a node..

I'm also seeing that you only use global $user so there really is no way for an admin to release other user's locks? Or maybe I'm wrong..?

I think requiring a permission to enforce locks is a little weird. It should be the other way around. You should only grant privileges to those who can bypass locks. Also, when trying to edit a locked node, I'm not given any warnings. The node just reloads, leaving the user very much confused. I also don't get any JS warnings when trying to navigate away from an edit page..

I know I'm throwing a ton of things into one issue, but these are all just coming up as I'm looking into the cancel button issue.

Attached is patch that only deals with making the cancel button an actual button, removing the non-supported stuff in _content_lock_add_cancelbutton(), handling cancel submits via the form api.

good job there

+ '#validate' => array('content_lock_cancel_submit'),

that should rather be

+ '#submit' => array('content_lock_cancel_submit'),

in addtion, i cant see a single point where you handle the redirect.

No it should be validate. Submit will only be called when the form is actually submitted, and that would process the node add/edit. You want validate so it jumps right the redirect regardless of any form errors, missing fields, etc.

I'm no longer doing a redirect because I removed the entry in hook_menu for node/%nid/canceledit. When the cancel button is hit, content_lock_cancel_submit() does exactly what hitting node/%nid/canceledit would do - remove the lock then redirect.

I hope these few changes and posed questions help. I've decided not to use the module so I most likely won't be continuing to patch things.

a i see.

Any alternatives to this module? ( i did not found any )

Iam currently really think about completely rewrite the module... i forked it from checkout and i find the architecture pretty weak. My goal would be to make locking a API, giving you the entity type ant the primary key, like node and nid. Submodules would implement locking for different system entitys like nodes, users or any other things.

I haven't seen any alternatives. The project that I considered this for wanted to do away with the locking functionality as the main concern was simply providing a cancel button. I simply just made on in a custom module.

What you're saying makes sense and sounds good.

I'd just like to note that places in Drupal core suffer this same bug, such as confirm_form().

Also, I did try fixing this my own way by upgrading <a /> to <button /> in javascript which avoids hacking around drupal: 0ec1f9790557c . Your patch, mikesteff, is probably much better though and I want to look at it when I can.

The checkout module has been fantastic but since it's on its way out I thought it time to try content_lock and immediately encountered the cancel button issue so I've downloaded the patch in #9.

As for the other thought in that comment to warn the user when leaving that is handled by the save_edit module and seems to work just fine.

The patch in #9 does not apply to the latest release. I see the patch is from September and the latest release is from November:

Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- content_lock.module        2010-08-29 21:40:08.000000000 -0400
|+++ content_lock.module.new    2010-09-20 11:49:34.000000000 -0400
--------------------------
Patching file `content_lock.module' using Plan A...
Hunk #1 succeeded at 65 with fuzz 2 (offset -6 lines).
Hunk #2 FAILED at 278.
Hunk #3 succeeded at 571 (offset 10 lines).
1 out of 3 hunks FAILED -- saving rejects to content_lock.module.rej
done
Press any key to continue . . .

Is it possible to get a patch against the current release? Thanks.

The patch in #9 fixes the Cancel button.
I have rerolled the patch from #9 against the 6.x-2.4 release.

subscribing

Committed, in commit 099299e.

Thanks very much for the patches and work at rerolling them :-).

I'm a bit confused ...

the changes will be in/when the next version (2.6) is released?

and/or available in the current dev?

or do I need to checkout?

Thanks.

@izmeez: You may check out the git version of content_lock if you need these changes now. The 2.6 release will hopefully be “soon” (whatever that means ;-)).

I'm not sure if there's a better bug state than “fixed” like “InVCS” which would make more sense with this workflow? Any suggestions?

I'm not sure what the best workflow is going to be with Git.

Before with CVS, some modules have -dev versions where the commits show up within 24 hours and then the maintainer determines when to make an official release. That way any users needing the fixes can use the -dev release while waiting.

Just trying to get a handle on things myself,

Thanks.