Internal nodes is a module that allows you to select content types that can not
be directly accessed by their URL (eg http://www.example.com/node/16).

When a user visits a node that is an internal node, they will simply get a
Drupal 404 error page. This will stop search engines indexing nodes that
shouldn't be viewed directly (eg web site banners) and so will help stop
duplicate content on your site.

Project Page: http://drupal.org/sandbox/mjpa/1136278
Git access: git clone --branch 7.x-1.x mjpa@git.drupal.org:sandbox/mjpa/1136278.git internal_nodes

Comments

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Needs review » Needs work

Module file is missing

Please fix the reported issues. Don't forget to set status to needs review after committed to your sandbox project.

Powered by Dreditor (triage sandbox) and Triage transitions

mjpa’s picture

mjpa commented
Status: Needs work » Needs review

The module file is there. It's in the 7.x-1.x branch (original post has the git clone command for it).

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

You're right. The code is http://drupalcode.org/sandbox/mjpa/1136278.git/tree/refs/heads/7.x-1.x

You should use http://api.drupal.org/api/drupal/modules--user--user.module/function/use... I guess?

if (user_access('access internal nodes')) {

is equivalent with

  global $user;
  $allowed_roles = user_roles(TRUE, 'access internal nodes');
  $intersection = array_intersect_key($allowed_roles, $user->roles);
  if ($user->uid == 1 || count($intersection) > 0) {

right?

I don't get the logic of

  // Not a direct request?
  if (isset($GLOBALS['internal_nodes_initialised'])) {
    return $node;
  }

hook_init is always fired right?

mjpa’s picture

mjpa commented

RE: user_access() usage - correct, don't know what I was thinking doing it manually, thanks - code has been updated.

RE: hook_init() - yes it's always fired but it is fired *after* the _load() function is called when processing the menu. There are times when Drupal will call the _load() function later in the page (and so isn't a direct request) so having the global variable allows me to check if it was a real direct request or not. Without that check there, edit pages produce a PHP warning and the 'view' tab URL is broken.

mjpa’s picture

mjpa commented

Couple of other changes based on a code review by someone on IRC - added a check_plain() for the checkbox values and changed the help text to work better for translating.

mrjeeves’s picture

mrjeeves commented

i like the module, clean, not much bloat, and a nice admin message to remind of hidden content.

mjpa’s picture

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

As I understand things, it's been reviewed / tested by 3 or 4 people now...

13rac1’s picture

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

Sorry. You can't set your own module to RTBC.

sreynen’s picture

sreynen commented
Component: new project application » module
Status: Needs review » Reviewed & tested by the community

Looks good to me.

13rac1’s picture

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

Hi! According to Joachim in http://drupal.org/node/1146410#comment-4545596 neither of our project applications can be "Fixed" until we determine the best method to handle the similar functionality of our modules. As you know, "There can be only one!" :D

My original purpose in creating Block direct view was to implement 301 Redirects and outbound URL rewrites to fix the Drupal search results for blocked nodes. I added 403, because it was a simple addition. It looks as though your motivation for Internal nodes was to return a 404 for blocked nodes. Normally 404 is returned when the data isn't available, but these nodes are technically available. Would 403s be a reasonable substitute?

I've looked through the Drupal core code, and realized there isn't a straightforward way to force a 404 for a valid node. Your replacement node_load() function works well. Could explain why 404 was implemented rather than 403?

Good idea with the node status messages! I implemented that plus a class on the node div today.

Hmm... Just realized, an issue with the use of 404; It will clutter the File Not Found logs if often used. Would make finding/correcting actual 404s more difficult.

mjpa’s picture

mjpa commented

The idea behind this module was to allow nodes to be created that, while obviously existing, don't exist at any URL so 404 is more appropriate. 403 to me would put the module in the role of a node access module which isn't what this is.

I did have a plan to add a redirect option where you could select a path to redirect to (per content type) which, for example, would list the nodes. So for example if you had a View listing 'news' but no actual 'news' pages by themselves, you would set them as internal nodes and redirect to the 'news' View. I was waiting for the module to get approved before I did this though to see if anyone was interested (i have no use case for it).

RE the logs, Drupal logs both access denied / page not found so either way 1 will get cluttered, but then if you're getting hits on internal nodes, you have something wrong - eg you're linking to them or they're indexed in search engines etc.

I've made a comment on your application issue page relating to your module (this isn't the place for comments on your module).

13rac1’s picture

13rac1 commented
Status: Needs review » Needs work

Just discovered, this node_load() replacement method causes PHP notices in core menu.inc for anonymous users:

Notice: Undefined index: tab_parent in menu_link_get_preferred() (line 2365 of /var/www/includes/menu.inc).
Notice: Undefined index: path in menu_link_get_preferred() (line 2369 of /var/www/includes/menu.inc).
Notice: Undefined index: path in menu_link_get_preferred() (line 2369 of /var/www/includes/menu.inc).

The menu.inc code starting at 2360 is:

    $item = menu_get_item($path);
    $path_candidates = array();
    // 1. The current item href.
    $path_candidates[$item['href']] = $item['href'];
    // 2. The tab root href of the current item (if any).
    if ($item['tab_parent'] && ($tab_root = menu_get_item($item['tab_root_href']))) {
      $path_candidates[$tab_root['href']] = $tab_root['href'];
    }
    // 3. The current item path (with wildcards).
    $path_candidates[$item['path']] = $item['path'];
    // 4. The tab root path of the current item (if any).
    if (!empty($tab_root)) {
      $path_candidates[$tab_root['path']] = $tab_root['path'];
    }

Perhaps menu_get_item_alter() could be implemented to fix? It might be possible to make menu_get_item() to return a workable $item?

mjpa’s picture

mjpa commented
Status: Needs work » Needs review

I can't get the php notices to appear for me, however notices are usually ignored on most setups.

Also, menu_get_item() from what I can tell returns FALSE for a 404 (ie when the _load() call returns FALSE) so if anything it's a core issue to fix

13rac1’s picture

13rac1 commented

As discussed, I implemented 404 in Block direct view using menu_get_item_alter(). More information at the Block direct view project application: http://drupal.org/node/1146410

sreynen’s picture

sreynen commented
Status: Needs review » Fixed

Now that mjpa is a maintainer on both projects, I've granted mjpa access to promote projects to full projects. I trust mjpa and eosrei can work out which project they both want to promote. Please do not promote both projects.

eosrei, now that mjpa can promote your project, I'm going to close your application. If you need to promote another project in the future, please open a new application and reference these so we can do it faster next time.

Thank you both for your patience with this process.

Status: Fixed » Closed (fixed)

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