The nodeapi example module was a big help to me. In the course of using it, I discovered some minor incomplete documentation in the .module file. One section, for hook_node_update, included some incorrect statements. I've attempted to correct that in the attached files. I also took the opportunity to edit the comments for clarity, add some information, and copy edit.

There are two files in the attached zip file:
nodeapi_example_notations.module: Includes my comments to explain the changes I made. These comments are marked with two asterisks (**).
nodeapi_example_edited.module: This is the clean file with edits.

CommentFileSizeAuthor
#40 examples.nodeapi_example_edits_1050632_40.patch8.07 KBrfay
#40 nodeapi_interdiff.txt6.41 KBrfay
#32 nodeapi_module_fix-1050632-32.patch6.08 KBjn2
#19 nodeapi_module_fix-1050632-19.patch6.01 KBjn2
#17 nodeapi_module_edit.patch5.94 KBjn2
#13 nodeapi_edited.patch7.42 KBjn2
#9 nodeapi_edited.patch7.52 KBjn2
nodeapi.zip7.19 KBjn2
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Thanks for the thoughtful work.

It's a lot easier for us if you provide a patch file - http://drupal.org/patch.

Would you mind?

jn2’s picture

jn2 CreditAttribution: jn2 commented

Newbie mistake. Been making a lot of those.

I looked into patches through the link provided and at least understand them better now. Well enough to know that on my Windows machine, getting set up to make a viable patch will take some thought.

jn2’s picture

jn2 CreditAttribution: jn2 commented
Status: Active » Postponed

Postponing until I'm set up for patches.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Thanks - It's not hard, so if you need any help, find me in IRC and I'll coach you. I use git, so make patches using http://groups.drupal.org/node/91424

jn2’s picture

jn2 CreditAttribution: jn2 commented

I'll have to read up on git first and set up a git client. Then we'll see if I might need some help on IRC. Thanks for the offer.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

jn2, the module passes tests and seems to work. The first edit of yours I found was the one to change FALSE to DRUPAL_FALSE, which yields an undefined constant error.

If you go to /admin/config/development/testing, you can select NodeAPI Example (under Examples disclosure triangle). Scroll down, click Run Tests and grab some coffee, and then later see what you hath wrought. :-) This is the same test that drupal.org will run automatically when you submit a patch.

It's fun to be a newbie. :-)

jn2’s picture

jn2 CreditAttribution: jn2 commented

@Mile23

Thanks for checking it out!

I'm a little baffled that you got that notice for DRUPAL_FALSE. The simple FALSE did not set the defaults for my D7 installation, so I checked out core and found this version. It works fine on my machine and in the installation in Quickstart.

Yes, being a newbie, there's adventure around every corner! ;)

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@jn2 - sorry I disappeared on you. Grab me when you see me. We'll get that working. Thanks for your contribution here.

-Randy

jn2’s picture

jn2 CreditAttribution: jn2 commented
FileSize
nodeapi_edited.patch7.52 KB

First try at a patch.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Postponed » Needs review

Hey, congratulations! Patch format looks great!

When you post a patch, set the status to "Needs Review". That makes the testbot run, and also tells people that you want them to review the patch.

Congrats and thanks!

jn2’s picture

jn2 CreditAttribution: jn2 commented

I'm glad you caught that so quickly, rfay. I didn't know the testbot needed the status "needs review" to run, so thought the patch didn't work. Thanks!

Status: Needs review » Needs work

The last submitted patch, nodeapi_edited.patch, failed testing.

jn2’s picture

jn2 CreditAttribution: jn2 commented
Status: Needs work » Postponed
FileSize
nodeapi_edited.patch7.42 KB

A few changes to see if this is the problem.

jn2’s picture

jn2 CreditAttribution: jn2 commented
Status: Postponed » Needs review

Changing status.

Status: Needs review » Needs work

The last submitted patch, nodeapi_edited.patch, failed testing.

jn2’s picture

jn2 CreditAttribution: jn2 commented

Back to the drawing board. I'll get this.

jn2’s picture

jn2 CreditAttribution: jn2 commented
FileSize
nodeapi_module_edit.patch5.94 KB

Next try.

jn2’s picture

jn2 CreditAttribution: jn2 commented
Status: Needs work » Needs review

Changing status.

jn2’s picture

jn2 CreditAttribution: jn2 commented
FileSize
nodeapi_module_fix-1050632-19.patch6.01 KB

Here's an all git patch made with format-patch against master.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Version: » 7.x-1.x-dev
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

Nice work, jn2. Definitely makes it clearer. A few comments below. 

+++ b/nodeapi_example/nodeapi_example.moduleundefined
@@ -49,7 +49,8 @@ function nodeapi_example_form_alter(&$form, $form_state, $form_id) {
+  //Here we check to see if the type and node field are set.

Need spaces between the slashes and the start of the comment, and the first line wraps too soon. It should go all the way to 80 characters.

Also, generally this type of comment would start with second-person imperative. In this case, that would be "Check to see if the type and node field are set."

+++ b/nodeapi_example/nodeapi_example.moduleundefined
@@ -71,19 +72,19 @@ function nodeapi_example_form_alter(&$form, $form_state, $form_id) {
+ * hook_node_build_alter()	-	Modify structured content after content built.

Whitespace problem. It looks like there is a tab before the hyphen. Also, should be "after the content is built"

+++ b/nodeapi_example/nodeapi_example.moduleundefined
@@ -71,19 +72,19 @@ function nodeapi_example_form_alter(&$form, $form_state, $form_id) {
+ * hook_node_info()	-	Define module-provided node types.

Whitespace problem. It looks like there is a tab before the hyphen.

+++ b/nodeapi_example/nodeapi_example.moduleundefined
@@ -105,8 +106,8 @@ function nodeapi_example_form_alter(&$form, $form_state, $form_id) {
+ * Check that rating attribute is set in the form submission, since the field is

Check that the rating...

+++ b/nodeapi_example/nodeapi_example.moduleundefined
@@ -188,15 +189,14 @@ function nodeapi_example_node_delete($node) {
+ * We can't simply update, since the node may not have a rating saved, thus no
+ * database field to update. So we first check the database for a rating.
+ * If there is one, we update it. If not, we call nodeapi_example_node_insert()

This should be more imperative, which is the general style of Drupal core.

"Since the node may not have a rating saved, there may be no database field to update. First check the database for a rating. If there is one, update it. Otherwise, call nodeapi_example_node_insert() to create one."

Having said this, in the comments in my own modules, I totally use "we", too ;)

+++ b/nodeapi_example/nodeapi_example.moduleundefined
@@ -188,15 +189,14 @@ function nodeapi_example_node_delete($node) {
-    // If may happen that this node does not have a previous saved rating

I think the original comment is clearer.

+++ b/nodeapi_example/nodeapi_example.moduleundefined
@@ -205,7 +205,7 @@ function nodeapi_example_node_update($node) {
-      // node has been rated before.
+      // Node has been rated before.
       db_update('nodeapi_example')
          ->fields(array('rating' => $node->nodeapi_example_rating))
          ->condition('vid', $node->vid)
-- 
1.7.1

This hunk no longer applies.

Powered by Dreditor.

jn2’s picture

jn2 CreditAttribution: jn2 commented

Thanks for reviewing the patch. I'd given up on it ever being reviewed.

Before I rework it, though, I'd like to discuss some of your suggestions. To begin with, you are right that second person plural does not belong in comments in Drupal core. However, this file is not core but from 'Examples for Developers', as much tutorial as module. The use of 'we' extends through most of the comments in these modules. I did a quick check of page_example.module, file_example.module and block_example.module, and they all use that approach.

If you read the opening comments for this file, you'll find that's how it begins. This is the first sentence in the @file comment: 'We will add the ability for each node to have a "rating," which will be a number from one to five.' So your style suggestions for the comments in this patch really move the patch away from being in line with the rest of the module.

On line 188, the gist of the sentence is conveyed in the edit to the doc block above. So I shortened the inline. Also, the inline was an incomplete sentence: 

    // If may happen that this node does not have a previous saved rating
    // value, so we can't just update it, we need to check first if this

That's the entire comment verbatim.

Really not sure what you mean by, 'This hunk no longer applies.' All that's changed is capitalization.

Thanks for pointing out the whitespace problems. Not sure how tabs got in there.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

IMO we should *try* to use standard commenting practices, but we don't need to be as rigorous as core.

It's far more important that the examples properly teach (and the comments teach with them) than the writer's first, second, third, or imperative style. So we can be a little more liberal, and I'd say "We" should be OK.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

'We' makes it easier to point out the narrative that's in the code. We gather the data, we check some boundary conditions, we process the data... It's much easier to talk about why we're doing something in code if we use that form. (See what I did there?)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

This hunk no longer applies means that that part of the patch fails when you apply it to the file. Hunks in patches fail when something has changed in that part of the file since the patch was created.

Sorry, I didn't mean to say that "we" never belongs in comments. I meant that it is often clearer if you don't use "we" (which just happens to also be preferred in core) and that in these cases, I think it is clearer without using we.

jn2’s picture

jn2 CreditAttribution: jn2 commented
Version: 7.x-1.x-dev »

Thanks to everyone for your opinions. I'm going to re-roll the patch, leaving in the 'we' construction and the corrected incomplete sentence, but including linclark's other suggestions.

Not surprising that part of it failed to apply. That patch was made in early March.

@rfay, I assume I should roll it against 8.x-1.x and change the Version?

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@jn2, it will need to be done on both, but it should be able to apply just fine either way.

TR’s picture

TR CreditAttribution: TR commented

@jn2: Right now there's no difference between the 7.x and 8.x branches except the core=7.x (or core=8.x) in the .install files. So you may roll the patch against either - it doesn't matter. This will change in the future as Drupal 8 starts to diverge from Drupal 7, but for now feel free to use Drupal 7 if that's easier.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Version: » 7.x-1.x-dev

I'd suggest not calling this a D8 module or D8 documentation. The point of this project is to have examples for how to code modules for Drupal versions that people would be running right now. Also, this is NodeAPI, which might well not make it into D8 anyway.

(This whole sense of starting with D8 when D7 is just out the door makes me itch. It's as if there will never be a D7.1, or even a D7.0.1.)

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

I'm itching too. Painful.

Oh well.

Let's avoid mentioning a Drupal version in comments anywhere in Examples unless it's absolutely necessary. I think there are mentions in dbtng_example, but ...

TR’s picture

TR CreditAttribution: TR commented

@Miles23: See #1139498: Port to D8 ! for a brief explanation of why I did the "port" to D8 and why I intend to support it. But basically, the way Drupal core works is that as of right now, ANY changes you want to get into core Drupal 7.1 need to be put into Drupal 8 first. That means if there's a bug in Drupal 7, it needs to be reproduced in Drupal 8, a patch rolled against Drupal 8, then committed to Drupal 8. After which it can be backported to Drupal 7. That creates a huge problem for people wanting to contribute to core - how can you reproduce a bug if there are no modules running in Drupal 8 yet? There are hundreds of outstanding problems in Drupal 7 core that have been pushed to Drupal 8 already - many of these are complex and don't show up except when a contributed module tries to use core. Porting "Examples for Developers" to D8 makes great sense to me in that context, because it provides working modules exercising many key features in Drupal which can be used not only to find and fix bugs but also to ensure that key functionality doesn't get broken by ongoing development in D8.

This is not all theoretical either - there's a bug in Drupal 7 that I'm personally interested in getting fixed that involves vertical tabs. This issue needs to be fixed in Drupal 8 first before it gets backported and fixed in (hopefully) Drupal 7.1, but how do you demonstrate the bug and test the fix? The vertical tabs example for D8 is the perfect way.

Anyway, I don't want to require we follow the same "D8-first" philosophy in this issue queue. I will take care of keeping the D8 port up-to-date. But I don't want to discourage people from chasing D8 head either, if they want, since that is how Drupal core is improved.

jn2’s picture

jn2 CreditAttribution: jn2 commented
Version: 7.x-1.x-dev »
Status: Needs work » Needs review
FileSize
nodeapi_module_fix-1050632-32.patch6.08 KB

Here's the new patch, a mashup of the original and linclark's suggestions. Since it doesn't matter, I patched against 8.x-1.x, since that's what I originally cloned.

TR’s picture

TR CreditAttribution: TR commented

This patch is going to conflict with changes made in #767204: API.drupal.org: Rework Doxygen comments so people can navigate the comments. Since that other patch is 3500+ lines long, I'd really like to avoid having to resolve the merge conflicts (again) if this issue is committed first. So what I propose to do is hold off on this current issue until #767204: API.drupal.org: Rework Doxygen comments so people can navigate the comments is committed.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

OK, we'll get that big doxy one in real soon now, and we can get this one out of our hair.

jn2’s picture

jn2 CreditAttribution: jn2 commented

Great! Thanks for the update, rfay.

Actually, looks like it has happened. I'll need to look at this patch again soon.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Yes, it has happened. We can get this one in this week. Thanks!

jn2’s picture

jn2 CreditAttribution: jn2 commented

#19: nodeapi_module_fix-1050632-19.patch queued for re-testing.

Oops, got the wrong one.

jn2’s picture

jn2 CreditAttribution: jn2 commented

#32: nodeapi_module_fix-1050632-32.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, nodeapi_module_fix-1050632-32.patch, failed testing.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
nodeapi_interdiff.txt6.41 KB
examples.nodeapi_example_edits_1050632_40.patch8.07 KB

Here's a reroll.... I do this by checking out a version near the date of the patch, applying the patch, then merging that into the current stuff.

Attached an interdiff too

jn2’s picture

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

Cool! I read it through and it looks good to me. RTBC.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Reviewed & tested by the community » Fixed

Committed:
7.x: b50ec03
8.x: f4fbd9f

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@jn2, may I make you a maintainer of Examples? I know you won't do anything you don't think you're ready for. See #1007276: [policy, no patch] Comaintainer practices and procedures

jn2’s picture

jn2 CreditAttribution: jn2 commented

Sure, that would be great! Thanks, rfay. I think Examples is one of the best parts of d.o.

Status: Fixed » Closed (fixed)

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