[D7] Node Auto Fieldtoc

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxthebruce2088839git

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.

Thanks for posting this module. I know I've built this manually a bunch of times, so it's good to see it produced in a repeatable fashion.

I noticed a typo in .info specifying path to module, missing an o: files[] = node_aut_fieldtoc.module

The .install file is missing. The install file could hold a hook_uninstall() that deletes the variables created by the module. Even though node_auto_fieldtoc_node_type_delete() will remove all values when a content type is deleted, the install hook should remove any and all vars that the module created, even if the content type remains.

I noticed the automated review asks for the #options on line 87/88 to run through t(). In order to remain multilingual-friendly, make sure to only translate the values and not the keys, as demonstrated in this patch.

I did a preliminary review of this on GitHub a few weeks ago so my follow up review is pretty short...

node_auto_fieldtoc.info

• Since you've marked panels as a dependency you don't also need to mark ctools, but doesn't hurt anything to be in there either :)

node_auto_fieldtoc.module

• In node_auto_fieldtoc_form_node_type_form_alter() you might consider adding a #states property to the node_auto_fieldtoc_field_fragment select since it doesn't make sense to select a value if the node doesn't support the feature.
• Well done on cleaning up after yourself if node types are altered or deleted. You sadly don't see that enough in Drupal.

node_auto_fieldtoc_pane.inc

• Will this ever happen since the plugin defaults have been set? If so, maybe you can pull from that variable instead, i.e.
  

      if (!isset($conf)) {
        $conf = $plugin['defaults'];
      }
    

  so you don't have to redefine the defaults here.

  You could alternatively just add the defaults without checking, since array addition won't overwrite existing values:

      $conf += $plugin['defaults'];
    

• In the context loop, what happens if there's more than one node context available to the current panel? I'm not sure if there's a reliable way to find the "right" one, but probably worth double checking.
• My previous point about sorting by weight might still apply here.
• You could move this variable outside of the foreach, but as I said before this should be handled by l/url automagically, so I'm not sure why you're looking up the alias here...

I wouldn't consider any of these issues to be blockers for granting git commit access. This is a well written module and thebruce is a talented and detail-oriented developer who absolutely would be a great addition to the d.o committers.

You might want to include a brief bit of info about Panels Custom Content Panes in the documentation, or perhaps a link to a tutorial:

http://fourkitchens.com/blog/2012/12/13/creating-custom-panels-panes-and...

http://www.pixelite.co.nz/article/building-custom-content-panes-aka-ctoo...

http://www.nicklewis.org/drupal/tutorials/ctools-custom-content-types

BTW, I also forgot to mention that I ran the module through coder and got no error ... thebruce FTW!

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

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: .../klausi/pareview_temp/plugins/content_types/node_auto_fieldtoc_pane.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   166 | WARNING | Do not call theme functions directly, use theme('item_list',
       |         | ...) instead
  --------------------------------------------------------------------------------
  

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:

• "'ol' => 'Ordered',": this is user facing text and should also run through t() for translation.

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

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

I would like to see a screenshot or something, I found it hard to imagine just what this would produce from reading the description alone. It's a extremely jargon-heavy.
I have to think quite hard about what "Adds automatic content type field anchors and a panels TOC pane." really means to a site-builder in practice. :-}

To be honest, I got as far as the instruction to "On a page where you have a node context for an enabled content type add a pane." and simply could not guess what that means. I know a lot of Drupal-isms, but I can't figure out what to do here. I suspect that "node context" is not referring to the context.module

Maybe I'm dull. Do I have to enable "Panel nodes" or "Page manager"? ...

OK, it seems that these instructions, and this module, requires "Page manager" to be enabled also, but it's not a requirement in the .info here.

With that turned on, I then enabled the Panels version of a "node_view" page.
I then created an alternative Variant layout for my nodes.
After placing some content into the panes, I was then able to continue following the instructions, and yes, it seemed to work.
I did not need to add any "context" myself, and I'm still not sure the docs are clear. Though I guess a 'node context' is implicit once you know what that means.

I found an error when trying to use the panel 'preview' tab
Home » Administration » Structure » Pages » Node Template » Variants » Preview
/admin/structure/pages/nojs/operation/node_view/handlers/node_view_panel_context/preview

Notice: Trying to get property of non-object in node_auto_fieldtoc_pane_render() (line 62 of .../node_auto_fieldtoc/plugins/content_types/node_auto_fieldtoc_pane.inc).

I now see what this module does. It's OK, but I'm puzzled that you needed to tie this in to requiring Panels to work. Could the TOC element not just be exposed as a field-like object on the $node and managed through normal field UI? That would make it available without panels, and also compatible with DisplaySuite, View Modes, as well as Panels pages still.

Anyway, the code is well-structured and readable. Looks very sensible to maintain. Uses Drupal API well in all places. This is good to go.

-----

Thanks for your contribution, thebruce!

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.