[D7] Members Page

klausi,

Thank you so much for the excellent code review. I see you have been a busy man.

I was waiting, and hope was fading. But step away from the keyboard for a few weeks, and look what's there when you return!

Once again, thank-you. Now that I see how it's done, perhaps I can pitch in and help like you. Success with your thesis, spirit, and career.

-Dio

P.S. -will update Notbot shortly

Cleaned up code per klausi's recommendations.

Added support for the popular Logintoboggan and Content Profile modules in the latest 6.x-1.x commit.

OK - cleaned up per code sniffer, added 8.x-1.x-dev branch, cleaned out master.

Try again?

If I may quote a famous American cultural icon…

DOH!

I was making an assumption based on what I was downloading. (note to self - Self: RTFM).

On the other hand - I am getting quite comfortable with git.

OK - back at ya. - SERVE!

And why is there no emoticon widget when you need one?

Thanks for the review Klausi, you have a good eye for code. You actually read the code, understood it, and made intelligent comments. A rare gift (or maybe curse). You could be a religious scholar. Maybe you are. I digress.

There have been a lot of revs and commits since your last review. A summary:

The project name has been re-branded to "Members Page" (from NotBot) and the module installs as "members" rather than "notbot".

Now a point-by-point of your review:

1. style information should be done in CSS files only. - This is a dedicated page that renders 2 different views - one with a login-block, the other with a user-menu. Typically both blocks are rendered within a sidebar. The members module renders both blocks by code, on a dedicated page, regardless of the current site settings. A 30% width for the drupal blocks works with almost all themes in this context.
2. require_once - I saw this comment and immediately thought "why is this code in there?" My only defense is that I did this after adding the module and failing to enable it. Stupid. Then I copied the code! Stupider! I promise not to do it again.
3. you should not call that function directly, use drupal_get_form() and any form_alter() will be applied also

   I read this and I thought "yeah, he's right". So then I tried it. It looked liked it worked so I checked it it. It didn't work (discovered upon testing).

   Submitting the form rendered with drupal_get_form() resulted in validation errors - none of the fields completed were valid and the page submit returned with a totally blank form. I tried to debug this but was overwhelmed by the drupal_render array that was returned.

   You are right Klausi, in that drupal_get_form() should be used. But this module was developed from the bottom-up rather than from the top-down. I can't figure out why this does not work (yet) but I ask that you make an exception here.

4. use menu_get_item() - done - thanks for the tip..
5. notbot_block_role() - gone - no longer needed with new design.

/**
 * Renders a drupal block without using block_page_build().
 *
 * @param $fields
 *   Associative array of block fields required to render the block.
 * @param $child_render_func
 *   A callback that returns #children - an HTML string.
 * @param $subject
 *   A string for the block title.
 *
 * @returns $block
 *   A block for rendering by theme('block', $block);
 * @see members_page()
 */
function members_render_block($fields, $child_render_func, $subject) {

Could someone explain why the errors reported above are being flagged? I honestly don't know what I'm doing wrong. I copied the format used in blocks.module (been looking at that file a lot lately).

I suppose I should RTFM on Doxygen again but ... zzzzz.

Ah - I see - thank-you patrickd!

Added new versions and contrib mod info.

Revised the Issue title

Now fully operational for versions 6, 7 and 8.

D7 version does not appear to work with the Fusion Accelerator module.

Thanks for the review and comments chrisroane.

1) Are you sure the cache needs to be cleared? I think installing a module does all the necessary work up front. As for changing the URL's after install, the submit function does a menu_rebuild(). I was hoping that might be enough. Anyways, clearing the cache is one of the first things I learned about installing modules. I figure if you have Drupal up and running, you already know about that.

2) I'm not clear on what you mean here. The message (or greetings) both appear on the same page, the difference being whether the user is logged-in or not. Maybe best explained with a picture. If you are an Admin (or whatever) you can edit both greetings on the Members Page itself.

3) You would not believe some of the dumb variable names I think up at first, so I do a lot of global search and replace (with verify). I even did a module name change from NotBot to Members Page (read the first sentence again to really understand me). It's really nice if it's all in one file. Hey - it's only 600 lines ;-)

4) Oh man, I just knew that would come up! OK - I have tried drupal_lookup_path(), drupal_http_request() and menu_get_item(). drupal_http_request() worked the best for D6 but failed in spectacular fashion for D7 & D8 in a real test scenario (where the default urls are taken). I have sought advice from the experts and even tested their suggestions until I was ready to scream. On the other hand the code and my expertise in drupal keeps getting better and better. I digress.

I wrote members_check_url_free() using the php curl library because it is simple, widely used, well documented and pure php. It does not rely on any Drupal code. And it works across 3 versions of Drupal. It is 13 lines long. Compare this with any of the above functions.

BTW - the members.inc file has one function used in both the .module and .install file. I had to use an include_once directive in D7 but D8 was happy with a simple include. Curious.

Once again, thanks for the comments. I really do need to add a picture to the project page. This is a shot of the dev system. I am having some fun testing. Don't worry, it won't be in the release code.

OK patrickd - I'm in a bit of a conundrum. I develop on a Win 7 machine. It has dual boot with Ubantu but I'm not quite there yet with Drush and all.

I have no idea how to replicate or debug what you have done. Can I ask you to try a conventional install rather than with Drush?

The crazy stuff you refer to simply increments the url (members, members2, members3, etc) until an available url (not already used) is found. That's all. I don't think it's near as crazy as the code in drupal_http_request(), menu_get_item() or even drupal_valid_path().

I have tested this in many different ways and with D6, D7, and D8 and with over 20 different themes on a Windows system and on a Linux shared host.

I keep being told I am not doing things the Drupal way. Know what? Sometimes the Drupal way doesn't work. I keep discovering this with modules that I try to get working in D7.

Sorry for the rant, but this gets the project pushed to the back of the queue and I can tell you now there is precious little I plan to change. I'm approaching 90 commits now and it has been tested pretty exhaustively, just not with drush.

@patrickd

I have replaced the reference of $_SERVER['SERVER_NAME'] with the Drupal global $base_url in both the D7 and D8 versions and have tested both in my test environment: WAMP stack - virgin database install with a Basic page and an Article already occupying the module default urls of 'members' and 'register/user' respectively.

Attached are screen shots of each install, with diagnostic messages that indicate that a successful install did happen.

Replacing a PHP global with a Drupal global may not be the best answer here (IMHO), but I believe it may remedy the problem you had with the drush install.

Thanks chrisroane;

1. OK - I'll add the "flush cache recommendation". I've never had the need to do this myself (just tested again on a live site) but I'll take your word for it.

2. Were you speaking of the location of the admin fields where you change the url's? They are located on this form (admin/config/people/accounts) because the behavior of the registration form is set by other existing fields on the same page (the registration/cancellation field set).

The module has a detailed help page that has links to the configuration pages for the module. I copied this same text to the readme file but the help text is always changing. I look into making it a bit clearer.

Or perhaps you did not know where the members page was? That's not immediately obvious. I could probably explain this better. Have a look at the help page. There are links on it that open a new tab but the links have no underscore so they don't really jump out. Any suggestions on improving the text would be appreciated. I really struggle with this kind of stuff.

4. ?? I'm not sure I follow your logic about connectivity. The code is testing to see if a url exists before it claims it as it's own. In other words, it's looking for an invalid url.

function members_check_url_free($url) {
  global $base_url;

  $url = $base_url . '/' . $url;
  $cptr = curl_init();
  curl_setopt($cptr, CURLOPT_URL, $url);     // set url
  curl_setopt($cptr, CURLOPT_NOBODY, TRUE);  // no body, just header
  curl_setopt($cptr, CURLOPT_CONNECTTIMEOUT, 2);
  $content = curl_exec($cptr);
  $info = curl_getinfo($cptr);
  $errs = curl_error($cptr);
  if (!empty($errs)) {
    echo "<pre>"; print_r($errs); echo "</pre>";
  }
  curl_close($cptr);
  return $info['http_code'] == '404' ? TRUE : FALSE;
}

This is simply a page request (headers only, no body, 2 sec timeout) for a url. If the url does not exist, then a '404' is and converted to a TRUE. This function is fast, simple, and efficient. Considerations of function overhead and performance don't really matter for an install and the Member Page urls will not be changed often in a real situation, maybe not at all.

You can have a url on a Drupal website that Drupal is completely unaware of (I have several of those on my own website). Those url's are off limits for the Members Page module and Drupal doesn't even know about them. It does not make sense to use Drupal functions to detect this conflict because they may not work. Let me be more blunt - in some cases they simply DO NOT WORK as one might expect.

The function I wrote originally had no drupal dependencies. Now it has one ($base_rurl) but I only added that in there because I thought it might fix the drush enable command that patrickd had trouble with. All those messages you saw were just diagnostic messages I added to verify what I expected. They will largely disappear.

Regarding my simultaneous D6-8 dev effort, I currently have a D6 and a D7 live site that have these modules installed. Since I have a multi-site, multi version development platform, testing a D8 install is not a big deal once you have figured out a decent test sequence. And Git fetch makes updates so easy.

Let me sum it up this way. I have a simple function (13 lines) that works (for me) across 3 major versions of Drupal and has no Drupal dependencies. It will detect a url on a Drupal site that is not registered in any way with the Drupal system.

Having a module dynamically set a url is a bit of a novel idea for Drupal. I don't know if it's been done before. Some people are having a hard time getting their head around this. I have even been advised this is wrong.

My original motivation for doing this was the web-bots that routinely look for a form at /user/register and fill it out. Almost all of these web-bots have valid e-mail address (thanks gmail.com! >:-( ). I was getting about 10 registrations a day before I tired of the experiment and just shut it off (Who can register accounts? - admin/config/people/accounts). The system would send an email - you will be approved just as soon as pigs fly, please be patient - just kidding - I wish I had done that - it was just the standard drupal wait for admin approval boilerplate.

Meanwhile, the site also has a menu item on every page that says Members. On the members page is a link to a page that allows immediate registration. Not one web-bot has been able to figure that one out yet.

Whew, long post.

Thank you so much, chrisroane, for taking the time to install it and poke around and for the generous feedback. Explaining it to you makes me better at explaining it the next time. Your time and effort is really appreciated and if I can ever do the same for you, I will.

-Dio

My apologies for taking 4 months to reply -- sometimes it takes some that long to recover from a random drive-by shooting code review.

Regarding your Note 2 -- it was me who last set the status back to needs review. A previous reviewer had mentioned that it did not install with DRUSH. I did respond to that issue, changed the code, and asked for a reply. I never heard back. Waited for 3 months.

Then you came along and repeated the same problems installing with DRUSH; with the module naming conventions; code constructs (they make you PUKE?), local variables in camelNotation, using _private functions and the recommended alternate drupal functions that you (and others) think might be a better choice. All of these points, except the last, have absolutely nothing to do with:

1) does the module work as described?
2) making it work better.

I waited 3 months for this?

On the last point above, I have tried every drupal function that was suggested. I wrote my own in simple non drupal php because the same code works in all versions of Drupal.

But we have established one thing for certain here:

THIS MODULE DOES NOT INSTALL WITH DRUSH.

I don't know why. I don't use DRUSH. And I don't care anymore.

I have committed new code. There are many changes. I can't wait for this ship to come in. There are still 'issues' with pareview but there always are. If not, just wait until the code gets reviewed. There will be new things.

If these reasons condemn this module to a perpetual 'needs work' status, then fine. This will be the last time I set the status here to 'needs review'. I'm cutting bait after this cast.

Fabianx and logicdesign, thank-you for the reviews and the support; bhosmer, thank-you for your interest.

My apologies for the delay. It has taken a while to get things sorted out. If I may cut to the chase here, there are two remaining issues:

1. initializing variables in the .install file
2. using the curl library instead of Drupal API functions.

Variable initialization

I have moved the variable initialization from the .install file to the .module file and simplified it for translation purposes. This is counter intuitive to my way of thinking and a compromise; but so be it. It required a handler and changes to 4 lines of code. Simple text shows up now instead of nicely formatted html. Otherwise the module functions exactly as it did before. I left the Drupal 6 version as is.

Drupal API vs curl

This module has a configurable hook_menu() url. This is a rather novel concept in Drupal. The url in this case is a system variable and it must be initialized to something. Imagine if it was nothing.

The Drupal API offers the following candidate functions that I either knew about or were suggested to me.

1. drupal_lookup_path()
2. menu_get_item()
3. drupal_valid_path()
4. drupal_http_request()

I tried them all.

When writing code, it is impossible (and ridiculous) to plan for all contingencies; but good practice to plan for some. On my own Drupal 7 website I have several url's that the Drupal CMS does not know about. This is quite easy to do. I made a special one just for this discussion...

http://sontag.ca/whenpigsfly/

Drupal does not know about this page. It's not in any drupal database table anywhere. Apache deals with this url before Drupal even has a chance to look at it. So any Drupal API that looks at the database is pretty pointless.

drupal_http_request() held promise and I really did try to make it work. But in the end I opted with a home grown function that made use of the PHP curl library.

There are two advantages to this. The function is very simple [13 lines for drupal_check_url_free() vs. 296 lines for drupal_http_request()] and the home grown version works with every release of Drupal (6,7 & 8).

In preparing this defense, I did a lot of testing. For any skeptics that remain, I have developed a couple of novelty modules to demonstrate how vulnerable Drupal is to url conflicts. Here are two new sandbox projects:

1. When Pigs Fly
2. Baboons Beware

The support page is here. For a quick install and test, download the package from either wpf-7.x-wtf.tar.gz or wpf-7.x-wtf.zip.

Review the code. Read the README file. Try it out. You may be surprised.

There are 3 levels of WPF url spoofing (I just made this up now)

1. contributed module url
2. url-alias
3. subdirectory/index.php injection

The current Members Page code, as it stands, will pass any WPF permutation on install. It will also elegantly recover on a reconfigure after any WPF permutation is installed.

Name one other Drupal module that can do that.

So that function in question -- drupal_check_url_free($url) -- stands as is. It makes the PHP curl library install a requirement. And a drush install will FAIL (unless there is something that can be done that I don't know about).

About this code review process

This project has been held up because it did not install with drush. There are all kinds of reasons why this is and it has taken this long time to get sorted; but in the end, this is NOT a good reason to withhold approval. Installation with drush is not a criteria for approval, nor should it be for all kinds of reasons.

I am quite happy to document that this module will not install with drush and requires that the PHP curl library be installed. Not a problem.

I only ask that this be examined in all contexts. Maybe this is a problem with drush. Maybe this is a problem with Drupal. Despite the changes, the code works almost exactly the same as it did 10 months ago as long as one did not use drush to install it.

-Dio

With all due respect, you are asking to add another step into the User Experience (UX). First the module must be enabled in /admin/modules and then another checkbox added to verify that the option selected is really what the administrator wanted when he/she/whatever enabled the module. Notice how many other modules do that.

What would Steve Jobs say? (I can't believe I just said that.)

This discussion could have been avoided altogether if no check was made at all, and a Drupal standard API (like drupal_valid_path) was used instead, which seems to be the preferred use-case with this Open Source Project.

I suggest that two versions be provided here. One that would install for drush users; the other would install for everybody else. On behalf of myself and the other 90% in the world that don't use Linux, my apologies for this inconvenience.

-Dio

P.S. will be seeking other opinions on this matter.

OK - I made the switch from windows to linux.

This latest version will install with drush. A directory called members_page is created after downloading from the git repository. The module can be enabled, disabled and uninstalled with the following commands:

drush en members
drush dis members
drush pm-uninstall members.

The module will install with or without the curl library enabled. If the curl library is installed, then an additional check is made that will detect if a requested url (one that Drupal has no knowlege of) is being used.

Note that the function drupal_valid_path() returns TRUE for any path prefixed with 'http://sitename/' regardless of whether it exists or not. This, apparently, works as designed, something else I don't really understand.

There are still pareview errors, but none of them impact readbility or performance. Some I can't seem to get rid of, others I don't quite understand, and some I simply don't care about.

Fixed?

So this module gets 2 reviews within 20 minutes after sitting in the queue for only a month? Is this a sign that things are improving?

@pagolo - Thank you for taking the time to do a manual review. Your comment on localization is a valid one but I consider it a feature request. I would be happy to implement this request if this module was approved and in use. At this point it's not. Allowing feature requests to a module waiting approval is just wrong - it adds unnecessary delays to a process that takes way too long already.

Also, you should give a way to change the output of the members_page() function.

The output of the members page can be changed -- I have no idea what you are referring to.

You could use a template and separate logic from layout.

It is true that some CSS styling rules are in the code, but this module generates it's own content that no other module or theme knows about. I have tested this module with over 20 different themes. It works with almost all those themes with just a few tweaks.

@musicalvegan0

Thank you for the review.

I'm concerned about this: "A visitor who completes this form will be registered immediately without admin approval (but still subject to email verification) regardless of the current site settings." This seems like it should be an optional configuration option and disabled by default as it has security implications. I think it being separate from the site's current settings is fine so long as there is a way to disable/enable it.

This works as designed. The user/registration url that is on every drupal site out there has become an open door to spambots. Almost every site now has automatic registration disabled because that's where the bots first look. The members page module provides an alternate url (that can be changed) to foil the bots. The standard urer/register url remains where it is and functioning like it did before. The members page provides an alternate url for registration.

An admin does not install and enable this module unless an alternate page is desired. You are asking for a two stage enable process here (install module, enable module, activate module) which simply adds complications to code, documentation and usability. I am going to disagree here. This is not a good reason to changes the status from needs review to needs work.

You still have quite a few issues according to http://ventral.org/pareview/. You should work to clean these up first.

There will always be issues with pareview. Every time I run it there is something new it complains about - though none of the warnings have anything to do with whether the module works or not, or performance, or readability. I started this project long before there was any pareview. The camel notation was used in 3 different versions (D6, D7, and D8). The variables that use the camel notation are local ones (private in OO terminology). I could change the offending uppercase letter to lowercase and it would pass pareview but diminish the readability. I could also add an underscore, but drupal has so many underscores that it makes it more difficult to parse some drupal hooks. The no camel notation is just a rule lacking in reason but loaded with subjectivity. - there are lots of contributed modules that use camel notation and nobody is demanding that they be changed..

And please don't get me started on having 81 characters on a line instead of 80, or have 3 spaces characters when 2 were expected, with the warning repeated for every offending line. I have indented some lines to make them more readable. So pareview knows better?

Pardon my age and experience here but I'm going to conclude with a YouTube clip from Monty Python and the Holy Grail...

Then shall thou count to three, no more, no less.
Three shall be the number thou shall count and the number of the counting shall be three.
Four thou shall not count nor shall thou count two excepting that thou then procceed to three.

Five is right out.

Pareview, while quite useful, has become Drupal's Holy Hand Grenade of Antioch for code reviews -- used far too often to deal with cute little bunnies.

Beware of those killer rabbits!

I am setting this to RTBC myself. I know that breaks the rules but the fact is that NOBODY in the last year has found anything that does not work in this module. But everybody manages to find something good about it.

At one point it did not install with drush. It took a long time to find out why (I used to develop on a Windows platform) and there was only one reviewer who was helpful on why a drush install would not work. The others just complained. I switched to Linux this year and it was pretty easy after that to find an elegant solution that was much better than any of those suggested demanded in the reviews to get an RTBC.

One thing that really bothers me about this process is that if an applicant challenges what a reviewer has to say (after a switch from needs review to need work), the chances are excellent that you'll never hear from that reviewer again. Nobody likes to admit they are wrong. Or maybe they have already been promoted so doing more code reviews is now their lowest priority.

I have been offered all kind of suggestions about using alternative drupal calls that would be more in keeping with the Drupal standard. I really did try to do that, but Drupal has a few issues. Those functions did not work in my testing, so I crafted some that did. I even attempted to prove where Drupal would not work but none of that mattered.

In closing, the most important thing about a module is that it works as described and the documentation that decribes it. That it works out of the box. If someone can't break it or breech the security, then it is RTBC.

No reviewer has the right to suggest alternatives, or request features and hold a project app hostage until those demands are met. But that is what is happening here.

And that is simply wrong.

Thank you for the review.

I am going to challenge you on 2 points here because that's all I have time for at the moment.

6) _curl_check_url_free(): why do you need cURL and cannot use drupal_http_request()?

There are 2 reasons:

• drupal_http_request() fails under certain use cases. It is possible to have a valid site url that
  a Drupal system does not know about. I have already demonstrated this.
• drupal_http_request() is either not available or consistent across versions 6,7 & 8 of the Drupal API

_curl_check_url_free() is a simple function (19 lines) that makes use of the php cURL library. It works with every version of Drupal unaltered.

drupal_http_request() in D7 is 203 lines long, invokes all kinds of other incantations and is hard to figure out.

Why is there a problem with cURL anyway? A standard php install adds it in by default and most shared service providers include it as well. It's unfortunate drush has an issue with it but that is really minor and has been fixed anyway.

5) members_node_view(): this is vulnerable to XSS exploits

I thought that might come up but I left it as is, for a reason. The input field you speak of is an administration field that should only have sys admin authority. I consider sanitizing this input to be over-the-top security that will serve little good and may only restrict an admin.

The Members Page module creates an alternative page to /user/register at an available url of your choosing. The /user/register url remains functional as before and can be used as a honeypot if desired.

I don't wish to restrict a capable sys admin from inventing a url that is really hard for spambots to adjust to, only to have it filtered out because some other mother knows best.

Hoping for a response soon.

I promise to start doing code reviews too. Kinda busy right now.

I'll commit to reviewing this application as long as you're still working with us.

Thank you for making that commitment. I think this should be a "Best Practice" for PA code reviews. In a master/apprentice relationship, the master teaches the apprentice. The apprentice can ask questions (like why) and the master should be able to answer those questions in such a way that apprentice understands why. If that fails, either the apprentice is not cut out to be a master or the master has an apprentice that is about to change the craft by challenging the way things have been done in the past.

There is nothing more frustrating in this process than challenging a review on a premise or assertion made by a reviewer, and then not recieving an answer.

Let me put it another way. A reviewer can do a lot of work in reviewing a module (or very little). There may be some excellent points made in the review. But if an applicant challenges a point made by a reviewer and the the reviewer never acknowkledges or responds to that challenge, then that review isn't worth a bucket of shit. It's just an opinion of someone who cannot be bothered to defend it.

Please consider this and discuss this with your collegues. There is much room for improvement in this process.

=================================

I will now address the blocking issues, point by point.

1) Remove .gitattributes and .gitignore

Agreed. But a little history first. I added the .gitattributes after someone decided that all files must end in unix style LF terminators (including the .txt files). I used to develop on a windows system. I now have a dual boot Windows/Linux system with a common NTFS partition with the Drupal code base.

I was using git for windows, uploading and downloading to the Drupal git repository WITHOUT A PROBLEM. This is because git elegantly handles the OS native mode EOL by default. So does almost every text editor I have used. It does not matter if it is LF, CR/LF or CR.

Then I run an updated coder in windows, and guess what, it complains about the README.txt file ending in CR/LF. My editor had an option to save with unix EOL, so I swiitched that on. Of course any file I updated and saved after that triggered a changed that affect the whole file and not just the line I changed. More havoc was created when I switched to back unix.

That mysterious .gitattributes files came from http://stackoverflow.com/questions/170961/whats-the-best-crlf-handling-s.... It solved most of the problems I had.

The arbitrary decision to enforce unix style EOL caused me and others to reconfigure things, download new plugins, get new files, that WERE TOTALLY UNNECESSARY because others, including the authors of GIT and most every other editor, already handled it without a problem. GIT, by default, handles native mode EOL conversions for the client. It does not matter what platform the developer uses, GIT converts any push to unix style EOL for the repository.

The unix only EOL rule added to the coder module WAS A MISTAKE. There were no problems it caused to begin with but there were many unintended conequences as a result.

I know enough about human nature to know this decision will probably never be reversed, but hopefully others will think about such "great ideas" and run them by the membership before SHOVING IT DOWN OUR THROATS.

Ooo - Long post - saving now so this effort does not go to that big bit bucket in the sky!

To Be Continued...

Continued ...

2) XSS problem

OK - I'll add in the filter_xss_admin() but your explanation as to why it is necessary is not very convincing. I'd prefer some better answers than "we try hard".

3) NAMESPACES
OK - I'll change these too. But this is the first time this issue has ever been mentioned on the defines. The function drupal_check_url_free() was named that way because Drupal does need a function like this if paths can be reconfigured. And NO -- drupal_http_request() does not work for this purpose.

4) Use theme(username) instead of $user->name directly

OK - I'll do this too. But this is also the first time this has been mentioned in any review which illustrates just how subjective these code reviews can be.

If you look at the standard D7 User Menu block, you may notice that it does not list the User name. D6 did. I thought it was a nice touch, so I added this to my module. Using theme(username) instead of $user->name add little value here. It may be the 'Drupal way', but calling this a blocking issue and delaying approval of this module for this reason is over-the-top political correctness.

I want to discuss the other issues raised but this is enough for now.

Will push an update in a day or 2.

Updated and ready for review. Sorry for the delay. I have very been busy with other things.

All of the blocking issues have been fixed and, for good measure, I also added a theme function, a template file and a CSS file to the module; removed the markup and style rules from the code; and changed the camelCase notation and function names to match the Drupal standard.

It was more work than I expected. The module produces the exact same html as it did 9 months ago, just in a slightly different fashion.

I have not added CURL to a hook_requirements() call because cURL is not required -- the module simply works better with cURL installed.

Other Issues:

"A visitor who completes this form will be registered immediately without admin approval (but still subject to email verification) regardless of the current site settings." - why disregard the current settings?

The Members Page module does not affect the behavior of the standard user/register page. This is by design. It is intended to be an alternative to user/register. The user/register page can be disabled, or enabled with/without admin approval as before. If the user/register page is enabled without admin approval, then there is really no point in having the Members Page module enabled. A warning message is posted in this case.

cURL isn't always installed, but you're free to use it. hook_requirements() is the way to check for it though. We recommend drupal_http_request() to most people as they often find it easier to use.

kscheirer - please read #35 and the first point on #53.

I can't speak for most, but not everyone finds drupal_http_request() easy to use. It's very different in D6 and not even available in D8.

I offer a very simple solution that works with all versions of Drupal. And I keep being told that is wrong, that I really should use a 200+ line function that does not work in some cases, but since it has drupal in the name, it must be good.

Maybe I'm just in the wrong universe. Maybe i don't believe in the right gods or have enough faith.

Maybe I just don't belong here.

Thank you for the RTBC kscheirer.

For what it's worth, I was impressed by how thorough you were in post #55, even if I initially just expressed my outrage. I admire your knowledge and pluck sir, you followed through like you said you would.

High regards,
-Dio