[D7] Access unpublished

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will 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" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

I didn't see anything wrong in the code or in the Pareview, but I don't really understand how I could use the module. Would I have to write my own module to use variable_set('hash_access_url_key', 'your_hash_key')? I think there could be more detailed documentation at least in the README.txt file.

I did a quick review and I have couple of points to make :

I saw on the admin config page, you are saving to variable table by your own submit handler. For that purpose Drupal already has a API 'system_settings_form'
where you can pass the $form variable and drupal will do all submit handling and saving to variable table for you.

On line number 62. I can see that you are using 'check_plain' which I don't think is required. You should be using it when you are sending the data to the browser to avoid XSS exploits.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2257711

Project 2: https://drupal.org/node/2208357

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Hi,

Thanks for your contribution, here is the result of my codereview.

Readme file doesn't make any reference to the setting form and lets the user suppose that only devs can use the module.

.info file and .module file header says that the module grants "access to view unpublished node for anonymous users" but this is not true as this only depends on hash_access_view_unpublished_node permission.

for good memory usage and performance, form and form validate handler should not be in the .module, also the page call back hash_access_configuration only wrappes a drupal_get form.
i suggest the following:
changement/addition of the folowing to the hook_menu

'page callback' => 'drupal_get_form',
'page arguments' => array('hash_access_url_param_form'),
'file' => 'hash_access.admin.inc',

and then simplly move the form login to the new admin.inc file preventing the form to be loaded in the memory on every page. This way wit can be loaded only when needed :)
as a consequence hash_access_configuration can be removed.

in hash_access_url_param_form the #title value should be wrapped in t().

in hook_node_view instead of :

$coolurl = url('node/' . $node->nid, array('absolute' => TRUE)) . '?' . $url_hash_key;
$url = $GLOBALS['base_url'] . $GLOBALS['base_path'] . 'node/' . $node->nid . '?' . $url_hash_key;

l() function can be used here and it will be more safe :)

in hash_access_check_node_access instead of $GET that is not recommanded for use in Drupal (i noticed the check_plain for safety), i suggest using the API function for this drupal_get_query_parameters

Thanks again

hi,
i reviewed to code again and didn't notice something to mention.
I installed the module and tested it. it works as expected.
thanks again
let's RTBTC it :)

Can you outline how this module is different from https://drupal.org/project/node_authlink?

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. See https://drupal.org/node/1587704, section 1.2.

Neat lightweight module, works as advertised.

Did a review and found few minor non-blockers:

Very minor things which should not block the project:

• When saving unpublished node, I get message "Use hashed link for direct access to view this unpublished node." twice.
• When saving unpublished node, the aforementioned message contains hash-link to the node, but the link has same color as the rest of message, so I had some trouble locating the link. Seems that anchor class "active" is the culprit. When clicked on link and user is directed to page with hash then the link is different enough. Probably related to l().
• On Configuration page there is "Hash Access setting". Suggest "Hash Access" which is enough or "... settings" in plural (although there is only one setting).

Nice to have / Feature requests (not blockers):

• Ability to set how long the tags are valid. May be general configuration option or per node option. For example if default is 5 days. Then after unpublishing (or last unpublished save) it remains accessible for 5 days. Or - the page remains accessible for 5 days after accessed first time by hash. Reason: the link may leak (to search engines, to email systems which scan links etc).

Needs work (minor):

• On project page please refer to D6 Access Unpublished as predecessor to your module.
• On project page please refer to D7 Node Authlink as similar module and how your's is different.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2257711

Project 2: https://drupal.org/node/2208357

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2257711

Project 2: https://drupal.org/node/2208357

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Oh, one more thing which did not appear immediately.
I use drush cron regularly to run maintenance tasks.
It outputs me randomly the following line:
Use hashed link for direct access to view this unpublished node. [status]
On production this could be very annoying.

I checked and confirm that things I pointed out earlier are fixed.

@ragnarkurm, an extra status message that is only shown to drush users isn't a blocker. We don't need the projects to be perfect. The main blockers are third-party code, security issues, and major API problems. The status message isn't a major API problem. I'm setting this back to Needs Review. If you think this is ready for final review by an admin, then set it to RTBC.

Thank you for clarification. I'm quite new in doing reviews and was not clear when to change 'needs work'. So instead it goes to RTBC.

3rd party code

None.

Security

I dont see any inherent problems.

In long run hash aging would improve security.

Also noted that the module theoretically may conflict with other modules that are based on hook_menu_alter() to overwrite any previous 'access callback'. I dont have any good solution in mind. Maybe admin can be notified via message or status report that 'access callback' has/had non-default value.

However, these two aspects can be developed later and based on module user's feedback.

Major API problems

No problem. There is no API code duplication and all API hooks are used properly.

Automated Review

Nothing; false positive.

Manual Review

There are two similar modules. There is an open issue in https://drupal.org/project/access_unpublished which has
gone unanswered.
https://drupal.org/project/node_authlink is similar in some ways. I have an issue w/ a patch in that queue which has
gone unanswered. I would say that we can proceed with this application due to unresponsive module maintainers. See #30 below.

The module is a little on the lean side, but is above the 5 function / 120 line requirement.

Normally, permissions use spaces instead of underscores.

hash_access_node_view() doesn't explicitly need to use the global $user; that is implied by default in user_access().

hash_access_node_view() isn't checking the $view_mode parameter (which is also the better name). This means that the
messages will happen in teaser mode, so there may be many on a view page. There are also occasions where $view_mode == 'full' isn't
page mode (and vice versa), and this hook may also be invoked at some unexpected moments. Look into node_is_page() instead.

Using hook_node_view() to add the link into the $node->content[] as a renderable entry, or using
template_preprocess_node() to add it as a $variable may be a better options, or addition. Also think about making the hash a proper property on the node entity so
it is available in Views, etc.

Use NODE_NOT_PUBLISHED instead of the constant 0.

(*) Overriding the access callback is not good. Your tests run before the node_access() check, so
other modules will
not have a chance to deny viewing the node. Doing something based on hook_node_access() is a better option, which will work within the node access system. See also hook_node_access().
This is a major API issue and a security issue, as it can result to unexpected node access behavior, so this is
a blocker issue. Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hashing things twice in hash_access_get_hash_from_nodeid() may be a little overkill. Also think about using drupal_hash_base64()
instead.

I am amending my review in #28.

You have a setting form, but you don't have a hook_uninstall() to delete the variable when the module is uninstalled.

I also need to retract my statement about proceeding with this application. I want one of the more experience admins to weigh in on whether this needs proceed with the abandoned module process.

This sounds like it should live as 7.x.1.x branch in the existing access_unpublished project. Please open an issue in the access_unpublished issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

@martin_klima, just a friendly hint:

The Drupal.org "project ownership" team tend to follow protocol like a Stalinist party secretary.

This means that before any transition of ownership can take place, you need to comply with every single step of the process in exactly the way suggested in Dealing with unsupported (abandoned) projects.

The comment you've already posted is probably not enough. When they say:

File an issue in that project's queue under the "support request" category stating your request to take over ownership or assist in maintainership. If you have a patch for the module, mention that as well, as it shows you are already working on the code. For the title, use something like "Offering to maintain PROJECT_NAME". By putting the project name in the title, you can catch the attention of people reading via the global tracker.

they actually want you to do exactly that.

Is this all I need to become the owner/maintainer of his project?

It is probably all you need to become the co-maintainer. According to the project page for Access unpublished, aberg is still the owner of the project. The statuses say "Actively maintained" and "Under active development". These settings is usually an indicator that the owner is not ready to hand over ownership, but that the owner is happy to have you on-board as co-maintainer to maintain a D7 version. You may ask aberg (nicely!) if he is willing to grant you ownership as well. But in my experience (as co-maintainer of three projects), project owners want to retain the ownership to their Drupal.org hosted projects, even if they no longer maintain them themselves. (I guess it is for bragging rights. :-)

Hash Access was the way for me to get through a one-time approval process to get permission to promote it (and my future projects) to a full project. Will I get granted the "Create Full Projects" permission?

I am not the right person to answer that, since I am not a git admin. But if you look at the procedures listed under the heading Abandoned Module Applications, there is at least nothing that bar you from being granted the "Create Full Projects" permission if you take (in-effect) abandoned project under your wings as co-maintainer and produces a high-quality Drupal 7 release.

If I understand this correctly, you are just going to use your Hash Access code as the basis for a Drupal 7 release in the Access unpublished namespace? In that case, there should not be any doubts about "how much code was written by the user applying". Just get the Drupal 7 version ready and update the issue summary to point to the Access unpublished project page and git repo, and ask for a review.

(Disclaimer: I am a rookie reviewer and may have gotten it all wrong. Perhaps klausi or another senior git admin can shed more light on this.)

OK, it looks like you're all set for an ownership transfer.

In that case, post a issue in the Drupal.org project ownership queue, with the title "Transfer ownership of 'Access unpublished' to martin_klima". Set Component: "Ownership transfer" and Category: "Task". After you've posted it, ask Aberg to comment saying it is OK with him (you may search the queue with the word "Transfer" to get an idea of this works).

Good luck with the project!

When you take over the namespace, make sure you do not destroy the history! After becoming the new owner, clone the existing (D6) repo and make sure it its branch name is 6.x-1.x. Then copy all your own stuff (suitably renamed) into that directory (so that the 6.x-1.x branch is overwritten with your files), fork a 7.x-1.x branch and push that branch to origin. I don't know your git-fu rating, but it is important, when taking over an old project's namespace, that the old branch is correctly tagged and can be extracted from the repo if necessary. (I actually think git will prevent you from shooting yourself in the foot by refusing to receive a push until you've got it right, but it is better to get it right from the beginning.)

@martin_klima, once this gets committed to access_unpublished, we can use that to finish up the application process. Set this back to Needs Review, and make a note up in the issue summary.

Well, you should have held back the release of version 7.x-1.0 until the 7.x-1.x development branch of Access unpublished has been through the normal review process here and been afforded the status "Fixed". There was no need to create a release at all at this stage, but if you wanted one, it should have been named something like "7.x-1.0-alpha1". (We can still go on reviewing the 7.x-1.x development branch tho'.)

Also, the name of your application has now changed from "Hash access" to "Access unpublished", and it is no longer a sandbox. In the issue summary, you need to change the following:

1. Title
2. Module name
3. Module sandbox URL (Should be "Module URL" and point to full project".)
4. Module git clone

Git clone command for the sandbox is missing in the issue summary, please add it.

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

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/css/access_unpublished.css
  --------------------------------------------------------------------------------
  FOUND 16 ERRORS AFFECTING 7 LINES
  --------------------------------------------------------------------------------
    5 | ERROR | [x] Whitespace found at end of line
   10 | ERROR | [ ] Line indented incorrectly; expected 2 spaces, found 1
   10 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   10 | ERROR | [ ] CSS colours must be defined in lowercase; expected #0071b3
      |       |     but found #0071B3
   11 | ERROR | [ ] Line indented incorrectly; expected 2 spaces, found 1
   11 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   12 | ERROR | [ ] Line indented incorrectly; expected 2 spaces, found 1
   12 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   16 | ERROR | [ ] Line indented incorrectly; expected 2 spaces, found 1
   16 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   16 | ERROR | [ ] CSS colours must be defined in lowercase; expected #ccc but
      |       |     found #CCC
   17 | ERROR | [ ] Line indented incorrectly; expected 2 spaces, found 1
   17 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   17 | ERROR | [ ] CSS colours must be defined in lowercase; expected #f8fff0
      |       |     but found #F8FFF0
   18 | ERROR | [ ] Line indented incorrectly; expected 2 spaces, found 1
   18 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 7 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

• access_unpublished_permission(): do not line-break translatable strings in t() if possible. Also elsewhere.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to patrickd as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, martin_klima!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.