userpoints_nodelimit.module needs cleanup

This one I don't get. Why is it "dangerous" since the form can't be submitted?

Maybe a better way would be to disable all of the forms so that the user has a nice visual that they can't do anything instead of filling everything out only to get to the bottom to find no buttons to submit.

everything else is awesome, thank you!!
-Jacob

I think it's not good to set to NULL a variable passed by reference, especially when there are other modules which can change the $form passed in, and which could have problems difficult to trace.
To disable all the fields of the form could be a good idea, but don't forget other modules can add more fields to it; if you want to disable all the fields in the module, you should write some code that doesn't simply disable some fields it knows there are in the form.

Anyway, it's not true the form can't be submitted; it's the module which wants to prevent the form to be submitted.

I agree with you in that setting all fields to NULL would make debugging difficult, however, showing all the fields and removing the buttons at the very bottom of the form is just really bad UI.

Preferably we'd just disable the page altogether and display a message stating "you're over your limit" or something like that.

But in the "disabling all the fields" idea, you could just ensure that the module is weighted last, go through all fields and disable them. This is especially true/important because CCK initially sets its weight high (most likely higher than this module).

That is not a problem. When the module is being installed, it can change its own weight in the {system} table, like here:

You can change the weight to any value sufficient to the module to be the last one called after any other ones which make a change in the same form the module is changing.

I never said it was a problem I was just pointing out that in order to properly disable all fields it needed to be last in line (i.e. heaviest weight).

I think the best U.I is not showing the form altogether.
next in line is disabling all fields
last in line is removing the last two buttons.

I mean no offense to your offers of code and I do understand the "code cleanliness" to it but from a user perspective its really frustrating to fill out a whole form only to find no way to submit it.

You ever get that test in school that asks you to do 20 things then the last task is "Don't do anything"? that is what this reminds me of ;)

I mean no offense to your offers of code and I do understand the "code cleanliness" to it but from a user perspective its really frustrating to fill out a whole form only to find no way to submit it.

In the code I read there is a line that calls drupal_set_message() like in:

(which is actual code in the module).

What the user would see it's a page telling him (right in the top part of the page) he doesn't have enough points to create the node, so he would know what is happening.
The message could be more explicit, and advise the user that there is no way to submit the form.

I would not like to be in the administrator of a web site that should debug a problem caused by a module which assign a NULL value to a variable passed by reference.

The choice is then to help the administrator, or to help the user. I think both can get an help by not setting $form to NULL.

I'm not arguing the point of setting the $form to NULL we're agreed on that.

I think its really bad UI to display a full form that a user could fill out even though they have absolutely no way of submitting it. Especially because (A) displaying dsm is an administrator decision which is usually turned off and (B) where to display dsm is up to the themer and not to the coder (i.e. it may not always be at the top).

I think the best method is to set the module weight high (above others) and disable all fields.

Personally I don't think the user should see the form page at all but rather a message (non dsm) stating that they can't create any more content at the moment.

There is not settings to disable a message from drupal_set_message().

As you can see from the documentation of the function, which includes the source of the function, it just queues the messages, which will be then displayed later.
The function is also called by form_set_error() which is used to show any errors a module finds when the user submits a form. If it's true what you say, then the user would never see an error, when he submits wrong, or incomplete, informations.

look I'm not getting into a pissing match over who knows Drupal better.

Using DSM here, showing all the fields and then hiding the submit buttons is plain bad UI. I won't support it.

Kiam

If you can hide the submit button, then you can hide the rest of the form too.

So, go ahead and hide all the form elements, except for one markup item that contains the message text, without dsm().

Submit a patch that does that and it will get reviewed.

Lengthy arguments will get us no where ...

look I'm not getting into a pissing match over who knows Drupal better.

Nor am I.

I just reported what for me is something that can cause problems in a module I am using. As you replied with something that doesn't seem right, I replied back what it stated in the Drupal API documentation.

If for you to show all the UI is bad programming (and I agree on that; that I proposed is just the simpler alternative to set a variable to NULL), then the only possible way to resolve the "problem" is to disable all the fields in the form, whatever the fields are.

The only alternative I found to set $form to NULL (which also removes all the array keys the other modules suppose are present) is to change the menu callbacks used by Drupal.
There is a global variable $_menu which is an array containing all informations about the menu callbacks. For what I can see the structure of that array is:

Changing those values when $path is equal to node/add/book, node/add/blog, i.e., would permit to change the callback called when presenting the page to create a book or a blog node.

I am open to any suggestion. All I am trying to do is to resolve an issue, not to show how good I am in programming.

-- Kiam@AVPnet

This the patch I propose. It doesn't contain the patch reported in #266767: userpoints_nodelimit.module should better document how to use it, so it should be applied before that.

Again there is no patch. Do you enjoy issue emails going back and forth or what?

Ok. Let us see what happens this time. Maybe the problem is that the file I am attaching has the same name of an existing one.

I am sorry for any problems I caused.

patching file userpoints_nodelimit.module
Hunk #1 FAILED at 18.
Hunk #2 FAILED at 23.
Hunk #3 FAILED at 117.
3 out of 3 hunks FAILED -- saving rejects to file userpoints_nodelimit.module.rej

Can you please make sure the patches work against what is in CVS before submitting them? This is eating too much of my time.

Can you guess? It's exactly what I did.

Still problematic ...

(Stripping trailing CRs from patch.)
patching file userpoints_nodelimit.module
Hunk #1 FAILED at 18.
Hunk #2 FAILED at 23.
Hunk #3 FAILED at 117.
3 out of 3 hunks FAILED -- saving rejects to file userpoints_nodelimit.module.rej

Please make sure it works against the DRUPAL-5--3 tag before submitting it again.

These issues are very annoying and distracting, and has become a time sink for me.

I downloaded the last 5.x-3.x-dev version, I changed the code in userpoints_nodelimit.module, and then I created the patch.
If there are any problems in applying the patch, then I don't really know what else to do, as I followed the same procedure I follow when I create any patch.

If that can be of help, I can attach the userpoints_nodelimit.module as I changed it.

I don't know why your patch does not apply.

Here is the way I would do it:

Do not download the tarball.

Checkout Drupal contrib (or a specific module) using the tag DRUPAL-5--3 http://drupal.org/node/321

Change the code that you checked out manually using an editor to modify the parts you want.

Then create a patch against what is in CVS using the instructions here http://drupal.org/patch/create

The changes would then be applicable to the version in CVS, for this tag.