Views URL Path Arguments

Hello heddn,

Thank you for great module.

1. Please add git link corresponding to non-maintainer like "git clone http://git.drupal.org/sandbox/lucashedding/1872408.git views_url_path_arguments"
2.Please review your module from coder module and see the automated code review http://ventral.org/pareview/httpgitdrupalorgsandboxlucashedding1872408git
3.I suggest you to use in file: views_plugin_argument_validate_url_path.inc at line: 19, 31, function is_object() to check variable is an object.

Cheers!

As a recommendation, do at least three reviews of other modules to accelerate your process of approval.

I will check when I come back home, but the code seems too short to be considered for approval, please confirm because and in a mobile smartphone and I cannot download the code just now. Using drupalcode.org shows really few lines of code, maybe I am wrong (again, I will check in a few days, I'm just traveling).

Thank you for your effort.

manual review:

1. project page: the use case is not really clear to me - maybe give an example?
2. Only 100 lines of code: This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

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

Automated review looks good except for the naming convention issue discussed earlier.

README and Project page are now both more descriptive and describe the purpose/use case of the module.

Multiple reviews completed by the author with good comments.

The only thing I think which definitely needs looking at is that in views_plugin_argument_default_url_path(),

    if (ctype_digit($arg)) {
      $return = $arg;
    }
    // Is it a path alias?
    elseif ($nid = drupal_lookup_path('source', $arg)) {
      $return = $nid;
    }
    return $return;

could be simplified to:

    if (ctype_digit($arg)) {
      return $arg;
    }
    // Is it a path alias?
    elseif ($nid = drupal_lookup_path('source', $arg)) {
      return $nid;
    }

although even with this, there is a potential scenario where nothing will be returned - does this cause views any problems if drupal_lookup_path returns false for any reason? Alternatively if we're always going to get something relevant back from drupal_lookup_path then could do:

    if (ctype_digit($arg)) {
      return $arg;
    }
    // Is it a path alias?
    else {
      return drupal_lookup_path('source', $arg);
    }

Shouldn't stop approval but worth considering:

In validate_argument() in views_plugin_argument_validate_url_path, there are a couple of node loads. I can't decide if these are necessary, or if there should just be a query against the node table to check if the argument is a valid nid - it doesnt need to load the whole node as it only returns TRUE/FALSE, which for some node types with CCK fields etc could involve expensive queries (admittedly statically cached). You may at least be able to cut out the first node load. Don't think it's a show stopper in any case, but interesting to think about.

Duplicate submission. Deleting
(I've done this a lot today. Comment button doesnt seem to be indicating its been clicked very well :-( )

default argument plugin - understand, the simplified return values look good though and make the code much easier, good stuff.

validate argument plugin - I think you still needed the second node_load (after elseif ($nid = drupal_lookup_path('source', $argument)) {) as now $node->nid and $node->title are undefined and will throw warnings/not set correctly (unless you're dispensing with that info?).

For the query against the node table when nid is numeric, we should use db_select for D7 for compatability against other DB backends, so your syntax would be something like:

$result = db_select('node', 'n')
  ->condition('n.nid', $nid)
  ->limit(1) // as you only care if there's one
  ->execute();

if (count($result) > 0)  {
  // then we had a valid nid
  return TRUE;
}

Here's a good answer on when to use db_query vs db_select: http://drupal.stackexchange.com/questions/1200/given-that-db-select-is-m...

Having said that, it does look like running through that list this use case is pretty ok for using db_query so its up to you. If you stick with db_query though, make sure to enclose the table name in {} i.e.

SELECT COUNT(*) FROM {node} WHERE nid = :nid LIMIT 1

I'd also add a limit, it won't make a massive difference as you're querying an indexed column with unique key on nid, but every little helps IMO.

Changes look good to me. Happy to mark this as RTBC and see if anyone disagrees.

Good work, and a good module.

manual review:

• views_plugin_argument_validate_url_path: shouldn't the node queries respect node access grants? See http://drupal.org/node/93737 . Not sure that is relevant for your case?

Could be a potential security issue, so I set this back to needs work to hear your opinion. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Very good, looks RTBC to me now.

heyho

I personally don't think that a hook_help() that provides just the same information as the module description ... is really helpful. Maybe you could provide more information? eg. A more detailed example how-to? it took a while until I really understood how to use it.

But there's nothing critical and also your reviews show that you are experienced enough to get approved!

Many thanks for your contribution and reviews!

I updated your account to let you 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 get 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.