Code clean-up & PHP4 compatibility

I'm ok with most of this. These are the dislikes and questions I have:

1. SQL statement style - I prefer the way the statements are currently laid out. Drupal SQL documentation does not define a standard for this. I realize that it is an unspoken rule for core and that is fine. There are areas that are specifically left out of the hard coded standards so that modules can have leeway to decide. Use of objects is another example of this. Prior to D7, use of objects for anything outside of standard class objects was not used. Now in D7 objects are finally being allowed for library base code and part of that is because of the traction of that in the modules arena. The multiline format is much easier to read and use and can be found in other Drupal standards like arrays. This also helps readability of the following db_query function.

2. What exactly is incompatible with PHP4? I have not seen any issues with PHP4.

3. ! operator - ! is an operator just the same as + - / * and . Most recently the . concatenation syntax was changed because it did not match the current standards. ! is no different and should have a space on both sides.

4. Why did you remove the debug functionality? I have used that to have users help me debug and the module is only going to be getting into even more crazy and odd territories of Drupal and other modules.

5. In filter form you replaced the breaks with returns. If this was a simple module that may be fine but filter_form is going to be used much more. With your method, if I wanted to add code below the switch statement I would have to re-format the whole block.

1. and 2. : Ok, I am fine with removing object chaining. My discussion in point 1 was over:

$sql = "SELECT ...";
db_query($sql);

vs

db_query("SELECT ...");

3. My argument here is that ! is an operator the same as those. I realize core handles it differently and I would put money on it someday that will change just as concat has. I am also fine with complying with that unspoken standard for core patches but in the module arena where we have a little more play to test the waters and push the boundaries I think that spaces should be used on both sides of a negation.

4. Agreed, debug functionality should and will not be in final stable releases but for testing releases as this module is still in high testing mode, debug is a great helper. Tests go a long way but they do not help you find incompatibilities in remote systems. Much of what can go wrong with this module is not by itself but through the collision of other modules, this is where tests can not tell you where the problem is unless you have had that problem previously. The quick debug I implemented allowed users to easily submit some text that would give me their configuration and several break points that would allow me to much more quickly know exactly where something is going wrong.

5. That is assuming that no code will ever run after the switch statement. Which, like I said, is fine for a module that doesn't use filter_form much but with BF filter_form is where we do the heavy lifting and will only be adding more in the future. That is why I don't like the returns in the switch because then if you add code after the switch it will never run unless nothing in the switch matches, which is not necessarily the result I would be looking for. This issue probably doesn't really matter much anyway, when 2.x comes around this area will be much different.

Thanks for the support and help with the module but I would not recommend the feature set to be put in core by a far shot. There are too may caveats and odd cases that are not handled or not handled in a way that would be good for all.

Ok, I conceded on all points except the switch and SQL reformat changes. Committed. Thank you.

Let me first say the I completely respect you as a Drupal developer. You have proven yourself several times to the community. And I thank you for all the hard work you have done. I also would love to collaborate with you or anyone that has some Drupal experience and a common vision.

However, I am getting the feeling that you expect me just to hand over this module to you instead of collaborating. You've submitted a couple patches and have not fully supported them. It sounds to me like you expect them just to be committed without issue or defense.

With this patch I would like to see text on what is being done in the patch and why. Saying you are just cleaning things up and submitting a huge patch is not enough. Please list what you have done and why.

Also, if I have a concern with the patch, I will bring it up and if you successfully uphold your side then I will concede. However, "Because I said." is not a successful argument.

While this module is far from core worthy and I have made no claim that it is, I find it hard to believe that it breaks

...all of the points listed on http://drupal.org/coding-standards as well as sub-pages.

I don't think that I properly conveyed my thoughts. Just glancing through this patch, which seems to have a lot of needless - + of lines, I see many changes that have no rules to my knowledge.

A couple examples are:

1. Changing "content type" to "content-type". Everywhere I see has no dash, including core menu items and titles even in the admin menu module (which is your module). While this change is not life changing, if you want to have it committed, I would expect a reason for it.

2. Removing the quotes around administer filters in a comment. Again, not a big deal but without a reason I can only come to the conclusion that you expect this module to conform to you instead of a documented standard or reasoning.

There are many more changes exactly like that. While am perfectly amiable to make such changes if there is a good reason, I am not fine with changing every line of code to match someone else's opinion without reason.

If there are documented standards or supportable traditions, please inform me and point me to the pages or code that supports your statement. I make no claim that I know everything Drupal and learn new things every day.