Project page: http://drupal.org/sandbox/teppi/1936024

GIT: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/teppi/1936024.git search_exclude_nid

Overview

I had in two different projects the use case where clients wanted to hide some special nodes from the search for various reasons. A common example is a node, which has an embedded view (for instance with the module "Insert view") with a listing of the child nodes. If the output of this node is cached the body content of this node would go into the search index. Then it can happen that a search for a certain string returns the listing page as well as the listed node on the listing page, because a part of the body content of the child node was displayed.

I know it's a quite special use case, but I had to do it already twice, so I thought it might be time to have a module for this. I only found a Drupal 6 module which does basically the same. I put some effort into the Admin UI to make the adding of nodes as simple as possible. The actual functionality of the module is a simple rewrite of the search query.

I'm looking forward to any feedback! :-)

Manual reviews of other applications:
http://drupal.org/node/1995954#comment-7421916
http://drupal.org/node/1996830#comment-7430060
http://drupal.org/node/1997826#comment-7430208

New reviews:
http://drupal.org/node/1993854#comment-7466078
http://drupal.org/node/1990340#comment-7471020
http://drupal.org/node/1987530#comment-7471224

Comments

swim’s picture

swim commented

Hey Stefan Lehmann,

In search_exclude_nid.install.
Just a suggestion but consider removing your persistent variables via variable_del as you only have a small number regardless.

In search_exclude_nid.admin.inc, line 27.

  • Your including Javascript in an odd manner, if possible please implement drupal_add_js. However moving & attaching the Javascript as a file to the form might be more applicable for this situation.
  • The variable 'search_exclude_nid_search_exclusion_nids' is used rather often, could this be a named constant?
  • Your doing a lot in your 'search_exclude_nid_search_exclusion_form_validate', would some of this better be moved to a submit function?

Nice module, thanks =).

eriknewby’s picture

eriknewby commented
Status: Needs review » Needs work

Hi Stefan,

This module works great! Thanks for this cool contribution.
Here are a few things I found while doing a manual code review:

1. search_exclude_nid.install: To be more standards compliant it's best to use db_delete() instead of db_query here http://api.drupal.org/api/drupal/includes!database!database.inc/function...
2. search_exclude_nid.admin.inc
- I see some strings that are still missing the t() function.
- Why is the link in $nid_add_link capitalized and not wrapped in t()?
- The UI is a tad bit confusing for the form. Initially I kept filling out the autocomplete field and hit "Save configuration". The NIDs weren't getting added because it wasn't clear to me that I needed to click the "+ ADD NID TO EXCLUSION LIST". My guess is this is why you capitalized the letters, but I don't think that's a good idea. This of course is more a UI/UX issue and not a functional issue, so not a blocker. But it would be nice to see a more elegant approach.
3. search_exclude_nid.info: the "configure" path is incorrect.

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented
Status: Needs work » Needs review

Hey there! Thanks for the review!

@ hapax legomenon:

  • I'm removing the variables with variable_del() now. I didn't even know, that this function exists. :)
  • I put the JS into a file and added it with $form['#attached'] to the form.
  • I'm not sure if the usage of a constant would be appropriate here. I mean it's definitely a variable, which might change. So I left it like it is, but if others have the same opinion I'm happy to change it.
  • I renamed the _validate function into a _submit function and added it as a submit handler to the form.

@ eriknewby:

  • I refactored the variable deletion like hapax suggested.
  • Yes you are right. The admin UI is a bit weird. I removed the link, which adds the node id to the list. Now the node id is added if somebody clicks the SAVE button. I tried for quite some time to capture the autocomplete select event, from the autocomplete text field, to add the node id as soon as somebody selects an item in the dropdown list. But as it turns out, it's not as easy as I thought. It looks like it even would require a core patch (srsly?). If interested have a look here: http://drupal.org/node/365241
  • I fixed the link in the .install file.

Thanks again for the reviews! :-)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » klausi
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: klausi » Unassigned
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

manual review:

  1. search_exclude_nid_search_exclusion_form() and search_exclude_nid_nodes_autocomplete(): this function does not respect node access grants and the permission necessary to access the page is not 'restrict access' => TRUE. So someone could be able to see nodes that are not supposed to be shown to that person. Solution: either restrict the permission or add the node access tag. This is a security blocker. And please don't remove the security tag, we keep that for statistics and to show examples of security problems. See http://api.drupal.org/api/drupal/modules!node!node.module/group/node_acc...
  2. "theme_item_list(array( ...": do not call that specific implementation, use theme('item_list', ...) instead.
  3. search_exclude_nid_search_exclusion_form_submit(): doc block is wrong, see http://drupal.org/node/1354#forms Same for search_exclude_nid_search_exclusion_form()

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

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented
Status: Needs work » Needs review

Thank you klausi for the review!

  1. I decided to go for the option to set 'restrict access' => TRUE on the permission. As there is no per-user list, the other option would not make much sense and in reality this is an absolute admin feature. I also want to keep the management of the list as easy as possible and multiple lists per user would only make the actual search exclusion settings intransparent.
  2. done.
  3. Oh yes, true. Changed it according to your given example.

I hope to get some additional reviews done during this week. Thanks so far! :-)

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented
Issue summary: View changes

corrected git clone statement

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented
Issue summary: View changes

added another manual review.

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented
Issue summary: View changes

added another link to a review.

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented
Issue tags: +PAreview: review bonus

Adding bonus tag again for another 3 reviews I did today.

cybernetikz’s picture

cybernetikz commented

manual review:

Nice module Stefan!

line 58: What is this checking for - if (!empty($excluded_nids)) where previous line is enough.
line 80: No point to use intval($nid) where you already have $node->nid

nonzod’s picture

nonzod commented

in admin/config/search/search_exclusion

I think is better to give a more specific name for id in li tag in to the section "List of excluded nodes".
Now is "node-NID", you can change it in something like "excluded-node-NID".

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented

Thanks cybernetikz and nonzod for your reviews.

line 58: What is this checking for - if (!empty($excluded_nids)) where previous line is enough.

Yep true, it's obsolete. I changed the format of the variable at some stage and that's a leftover of the refactoring. Thanks!

line 80: No point to use intval($nid) where you already have $node->nid

True, I use $node->nid now.

I think is better to give a more specific name for id in li tag in to the section "List of excluded nodes".
Now is "node-NID", you can change it in something like "excluded-node-NID".

Hm, ok. I think it's already specific enough, as the outer form already provides the id "search-exclude-nid-search-exclusion-form", but to make you happy I added "excluded-" to the <li>'s id as well. :-)

Thanks for the reviews guys.

nsuit’s picture

nsuit commented
Status: Needs review » Reviewed & tested by the community

Worked great for me. I used node_access in drupal 6 to achieve what your module is doing, but this or the module for drupal 6 you mentioned would have been so much easier.

swim’s picture

swim commented
Status: Reviewed & tested by the community » Needs review

Not just yet. Changing status to needs review.

Hey Stefan,

In search_exclude_nid.admin.inc, line 35 - 40.
I see your selecting all the nodes which are to be excluded. Maybe this would be more appropriately placed in it's own function, simply returning nid & title of said excluded node(s). This means other devs could reuse this function if need be & it keeps your form function nice & clean.

In search_exclude_nid.admin.inc, line 71.
The description hints at a check,

This codes makes sure, that only actual node IDs are saved.

Possibly implement a validation function for your form as well as the submit callback doing exactly this. E.G. return a form error if something other than an nid is submitted.

The use of the JS is still a little unfamiliar to me as I could see similar functionality achieved with $form_state['rebuild'].

Cheers,

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

manual review:

  • "'title' => 'List of excluded nodes',": all user facing text must run through t() for translation.

The objections in comment #12 also don't seem to be application blockers, and since this was RTBC already ...

Thanks for your contribution, Stefan Lehmann!

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:

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.

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented

Thanks guys, I will apply your suggested changes, apart from the $form_state['rebuild'], as this probably means quite a lot of refactoring. But I'll have a look tomorrow.

I'm definitely keen to review further projects, if I have some spare time. :-)

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented

PS: @hapax legomenon, regarding the validation function. I want to make it as simple as possible. That's why I filter the user entries rather, than giving feedback about wrong entries. If people complain about this behavior I'll refactor it, but for now I'd like to leave it like it is. Thx.

swim’s picture

swim commented

Ahh sorry for bumping this xD.

Of course Stefan mine were just suggestions & your right validation can often get in the way & become a hindrance.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added another review