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

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  
  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/notbot.install:
+8: [minor] Format should be * Implements hook_foo().
+13: [normal] The $text argument to l() should be enclosed within st() so that it is translatable from within the install.
+13: [normal] Use st() instead of t() in hook_install(), hook_uninstall() and hook_update_N()
+13: [normal] The $message argument to watchdog() should NOT be enclosed within t(), so that it can be properly translated at display time.
+14: [normal] Use st() instead of t() in hook_install(), hook_uninstall() and hook_update_N()
+14: [normal] The $string argument to t() should not begin or end with a space.

Status Messages:
Coder found 1 projects, 2 files, 5 normal warnings, 1 minor warnings, 0 warnings were flagged to be ignored
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  
  FILE: ...pace/drupal-7/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  55 | WARNING | Line exceeds 80 characters; contains 81 characters
124 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...ace/drupal-7/sites/all/modules/pareview_temp/test_candidate/notbot.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
4 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: .../drupal-7/sites/all/modules/pareview_temp/test_candidate/notbot.install
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
  6 | ERROR | Whitespace found at end of line
  8 | ERROR | Expected 1 space(s) before asterisk; 0 found
  9 | ERROR | Expected 1 space(s) before asterisk; 0 found
11 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
11 | ERROR | Expected 0 spaces before closing bracket; 1 found
12 | ERROR | Inline comments must start with a capital letter
12 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
    |       | question marks
14 | ERROR | Concat operator must be surrounded by spaces
16 | ERROR | Whitespace found at end of line
17 | ERROR | Whitespace found at end of line
18 | ERROR | Whitespace found at end of line
23 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------


FILE: ...e/drupal-7/sites/all/modules/pareview_temp/test_candidate/notbot.module
--------------------------------------------------------------------------------
FOUND 50 ERROR(S) AND 1 WARNING(S) AFFECTING 37 LINE(S)
--------------------------------------------------------------------------------
   4 | ERROR   | Trailing punctuation for @see references is not allowed.
   4 | ERROR   | Trailing punctuation for @see references is not allowed.
   5 | ERROR   | Whitespace found at end of line
   7 | ERROR   | Whitespace found at end of line
146 | ERROR   | Whitespace found at end of line
155 | ERROR   | Whitespace found at end of line
162 | ERROR   | Whitespace found at end of line
163 | ERROR   | Whitespace found at end of line
171 | ERROR   | Inline comments must start with a capital letter
171 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
171 | ERROR   | Whitespace found at end of line
173 | WARNING | Line exceeds 80 characters; contains 99 characters
173 | ERROR   | Inline comments must start with a capital letter
173 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
173 | ERROR   | There must be no blank line following an inline comment
173 | ERROR   | Comments may not appear after statements.
174 | ERROR   | Whitespace found at end of line
175 | ERROR   | Inline comments must start with a capital letter
175 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
178 | ERROR   | Inline comments must start with a capital letter
178 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
178 | ERROR   | There must be no blank line following an inline comment
180 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
181 | ERROR   | Concat operator must be surrounded by spaces
185 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
186 | ERROR   | Concat operator must be surrounded by spaces
189 | ERROR   | Whitespace found at end of line
190 | ERROR   | Comments may not appear after statements.
190 | ERROR   | Inline comments must start with a capital letter
191 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
191 | ERROR   | Comments may not appear after statements.
210 | ERROR   | Whitespace found at end of line
222 | ERROR   | Whitespace found at end of line
232 | ERROR   | Inline comments must start with a capital letter
232 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
244 | ERROR   | Whitespace found at end of line
245 | ERROR   | Inline comments must start with a capital letter
245 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
253 | ERROR   | Whitespace found at end of line
257 | ERROR   | Whitespace found at end of line
258 | ERROR   | Whitespace found at end of line
261 | ERROR   | Whitespace found at end of line
262 | ERROR   | Whitespace found at end of line
265 | ERROR   | Whitespace found at end of line
266 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
276 | ERROR   | Whitespace found at end of line
279 | ERROR   | Whitespace found at end of line
291 | ERROR   | Whitespace found at end of line
299 | ERROR   | Whitespace found at end of line
302 | ERROR   | Whitespace found at end of line
303 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

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.

please don't block deeper reviews because of little formatting errors ;-)

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!

MANUAL REVIEW FOR 7.x-1.x
-------------------------

1. A fairly minor issue, but it would be worth mentioning in your installation instructions that they will need to clear the drupal cache in order for the new url's to work on the site. It also may be worth mentioning this on the config page (letting them know that if they change the path, the cache should be cleared).

2. Is there a reason why they admin area for modifying the message for members/non members is not in a separate configuration area? I didn't see an explanation for this in the readme or installation instructions.

It might be more straightforward to put all of the configuration setttings into one area that you can then define in the .info file. That way it is very obvious where everything is for this module.

3. This isn't totally necessary, but it would make the .module file more simple if you moved the admin functions to a separate file. I don't think this is a requirement...just a suggestion. :)

4. members.inc: members_check_url_free()....I understand what you are doing with the code to validate the path. However, why couldn't you use drupal_lookup_path() to accomplish the same thing? You could run the function twice to see if there is a source path or alias with that value already being used.

I went through the code and the items listed above are the only things that came to mind. However, since these are not major things, I'm keeping it set to needs review.

AUTO REVIEW for 7.x-1.x
-----------
- No errors via the Coder module.

- Ran the online PAReview tool. There are a few minor errors that came up, that should be easy to fix: http://ventral.org/pareview/httpgitdrupalorgsandboxdiogenes1234042git-7x-1x

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.

@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.

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

please visit http://ventral.org/pareview/httpgitdrupalorgsandboxdiogenes1234042git for the list of errors being generated.

Review of D7 Branch:

To start, I think this module is a great idea and will be excited to try it out once it gets moved into production.

I don't think there is a rule that says your module folder name has to match your project name, but this is a pet peeve of mine. If I download a project called "members_page", I expect to be able to enable it by calling "drush en members_page". Also, it appears that theres already a members module, but it's long been abandoned so I don't know how that works with your module being named the same.

Note: Your module is coded differently than others that I've reviewed. The thoughts below are based on my experience with looking/working with other Drupal contrib modules.

Note2: This module was set as "needs review" when it should have been "needs work" some of these items have already been mentioned previously.

members.inc

There are a few coding standards issues that paraReview.sh should help you fix.

When enabling your module via drush en members I get the following echoed out in an endless loop:

<pre>connect() timed out!</pre>

I believe this is because your using $base_url which I don't think is available from drush unless it's set in the settings file (which I rarely do).

Would it be better to use the following instead of your CURL method? http://api.drupal.org/api/drupal/includes!path.inc/function/drupal_lookup_path/7

members.install:

As far as I'm aware, hook_install() should only be used to setup database schema's. I'm not sure why it's being used to define variables.

You're defining constants (MEMBERS_ANON_GREETING, MEMBERS_MEMBER_GREETING) to set up markup. I'd suggest some other way to create this markup so that they can be translated. I think it would be best to create template files. I see where these items can be overwritten in the settings pages, but at the least I think it would be best just to set the variables without defining constants.

The "while (TRUE) {" statements make me wanna barf :) it seems like drupal_lookup_path would clean up a lot of code in this module

The hook_uninstall is great!

members.module

Line 14:

The help information seems to be the same as the README.txt. Is it necessary to add this and is this the best method?

Line 102:

I'd recommend using t() for translation in your hook_menu title and descriptions.

Line:168, 189, 210 & 218

Follow coding standards when declaring functions (members_editAnonGreeting).

line 230:

What is the reasoning for setting class='node'. If users have the ability to edit the markup, then they should be able to declare their own styles and divs.

Line 259:

Do not use Line 310:

Functions with leading underscores usually represent "private" functions and should not be called from other modules (_block_render_blocks)

I'm aborting this review at this point. I feel that there are too many updates/adjustments that need to be made to get this module into a reviewable state. I can tell you've spent a lot of time on this module and am not putting it down, but there are certain guidelines for drupal development that I feel haven't been followed in the 7.x-1.x branch of this module.

Dear Diogenes,

I've been reading the reviews, installed your module and tested it.
As I understand one of the objectives of those reviews is to make the contributed modules work better.

The remarks are not there to annoy you but to improve your module's quality and that requires an active participation from both sides, the contributor and the reviewers.

From what I see there have been some remarks on this project, and some of them have been ignored, as using API to check a menu availability instead of making a call to curl_init().
(wich is why it your module doesn't work with drush).

Notice that not all servers have php curl installed and using this function is not advised, as it can lead to incompatibility to certain configurations.
Why use curl when there are functions on the API that are compatible with all server configurations.
Beside that it makes your module incompatible with drush.

Concerning the check of a physical file on that address (as having a /login-user/index.html file on the server), that is not much to be worried about as the url is converted by mod_rewrite, and in fact you are all the time accessing /index.php?q=login-user/ so the index.html will never be a problem.

Beside that I see that your module uses the constants in a strange way... storing long html content, that by the way is not using t().
Those constants are just called once, so why create a constant at all and not just concatenate the HTML code with some t() ?

From what I see following the advices of the reviewers leads to a (more or less) fast approval.
Trying to keep non standard code that is potentially problematic/dangerous leads to looong wait times.

What proves that the review system works as a quality/security filter.

Beside all that, I think your module is really near to get approved, from my point of view there are those two points left (the curl to be replaced with the proper API functions and to change the constants by properly formated html with t()).

Best regards,

Sergio

@Diogenes, are you interested in continuing in the application process?

For me to RTBC this, please now only change:

* Add a variable to enable the modules functionality
* Check for this variable in your menu() function and return early if FALSE (default)
* Remove the check from the install file
* Add the "Enable Members Page" checkbox to the configuration form
** Use #states property to show the rest only if enabled: http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.html/7#states
* On the form submit to enable it, validate if the URL is taken like you already do and only allow changing of the URL - if that works.

That should be a quick change, improves UX, allows disabling of functionality easily and temporarily and removes all logic from install.

CURL can be left in easily, no problem with that.

:-)

The problem is that the install part is a very bad environment to use API functions.

It is not a full blown drupal environment, but rather a limited bootstrap environment.

Securepages uses this approach.

Other alternatives:

* Ignore the path problem all together and trust that admins change the path - if it is taken (in 90% the cases the problem won't occur)

or

* Use $items['my-module-name/register'] as _one_ standard path
* Register the user supplied path in hook_menu_alter instead by moving the path around and check if it taken
** Maybe add message if its not taken

Both things are way easier (now also in terms of DX).

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (found some problems with the project) or "reviewed & tested by the community" (found no major flaws).

Link to the project page and git clone command are missing in the issue summary, please add them.

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Diogenes,
I tested your module; it seems to work fine and be useful.
- automatic review: please test your module with coder and/or navigating to ventral.org; particularly, avoid camel case names and use lowercase and underscore instead.
- manual review: members_page() function doesn't support localization: it should be relatively easy to implement it. Also, you should give a way to change the output of the members_page() function. You could use a template and separate logic from layout.
I wish you a good job...
Paolo

Forgot to change application status to "needs work."