Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Status: Active » Closed (cannot reproduce)

No description, so I'm assuming this wasn't meant to be filed. If it was meant to be filed, please reopen and describe the issue you are seeing.

chriscohen’s picture

Status: Closed (cannot reproduce) » Active

Sorry, I'm not sure how, but the description is lost! (Maybe I should file a bug for that separately? ;)

The description's here again, anyway:

The documentation mentions $types, but does not explain how to specify what $types are. If a module is implementing this hook, how is it going to specify the $types that it wants to interact with, given that this is a hook, so the module can't control how it's invoked.

The documentation recommends reading nodeapi_example.module for an implementation, but the signature for the function in nodeapi_example_node_load() is actually ($nodes, $form), and not ($nodes, $types) at all, which is confusing.

jhodgdon’s picture

Title: Documentation problem with hook_node_load » hook_node_load doc needs more details on $types

RE: losing data:
#1112110: Preview resets information entered, if issue filing URL has query strings
It looks like two people had trouble with this in the last few days. I've bumped the issue, so maybe we can get it fixed sometime soon.

Anyway, thanks for reporting this! I think the fix should be:
a) Better doc for the $types parameter.
b) Add to the example so that it uses $types as described in the description of the hook (i.e., check types against the module's types and do an early return if it doesn't match a type of interest).

As a separate issue, nodeapi_example_node_load() in Drupal 7 has the wrong signature. I've filed this as an issue in the Examples project.
#1187560: nodeapi_example_node_load has wrong function signature

Mile23’s picture

Status: Active » Needs review
FileSize
2.36 KB

Looks like the nook_node_load() docs contain all the useful info... But here's a bit of an edit for clarity.

Will look at the Examples issue.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

I don't like the idea of moving the narrative description down. And any description of particular parameters should go into the @param area for that parameter, not in the narrative.

Also, I forgot to move this to 8.x. The patch should be made against the 8.x branch, then ported (if necessary) to 7.x (most patches at this time apply to both).

Mile23’s picture

FileSize
2.74 KB

OK, redone. The original issue here was: What does $types do? The @params should explain what it is, and the 'narrative' should explain how to use it.

Also added a reference to $types in the sample code, and did a little cleanup on hook_load() for clarity.

jhodgdon’s picture

This looks like pretty much the same patch as the last time. Again, can we leave the original narrative paragraph explaining what the patch does at the top, and if you want to have specific paragraphs about the other parameters, please put them underneath?

Also, just because someone requested adding more information about parameters doesn't mean it should go into the generic narrative. Please put the additional information about parameters into the @param sections.

Mile23’s picture

Is there some reason this function description should begin with an unreadable paragraph about other functions?

jhodgdon’s picture

If it's unreadable, someone should revise it. But the information tells how this hook is invoked, and where in the node loading process.

Mile23’s picture

Yes, exactly. There is no way to revise it to make it more readable without consuming a huge amount of space on the page, and that same paragraph is repeated within many other docblocks for other functions in that file. Therefore, I put something readable at the top, in order to convey useful information other than the loading cycle, which I judge to be very useful information but not absolutely vital to understanding the function. My initial readable paragraph explains the $types parameter, *in addition to* the @params explanation at the bottom.

There is no where else to go with this, in terms of brevity, readability and the information desired. You are welcome to ignore my patch if this isn't adequate.

jhodgdon’s picture

I'm sorry that you don't understand that first paragraph. There is one error in there -- field_attach_node_revision() should be field_attach_load_revision(). But other than that, it's an accurate statement of the sequence of node loading. Those paragraphs were added to all of the node hooks in an earlier issue, and reviewed by the community. I'm not sure how to make them clearer without expanding them to include a lot of background information that is hopefully on http://api.drupal.org/api/drupal/modules--node--node.api.php/group/node_... or in the drupal.org documentation.

Mile23’s picture

"I'm sorry that you don't understand that first paragraph."

No need to be snarky.

It's a bit of a waste of time to try to read that super-dense paragraph only to realize it doesn't contain the information one is seeking. Thus, I moved it to the end, where the user who actually is seeking its benefit can enjoy breezily scanning the preceding paragraphs to find the proverbial pot of gold at the end of the rainbow. Simultaneously, this leaves the joy of discovery available to the non-hook-sequence-seeking masses, with considerably reduced frustration.

If it is our job to kill rainbows, then by all means tell me why the ordering is important and I'll roll another patch. That's all I've wanted.

jhodgdon’s picture

My sincere apologies - I was not trying to be snarky, and I'm sorry it sounded that way.

So. I still don't like the order of information that you proposed in your patch, but I also see your point about the offending paragraph about when the hook is called.

How about this:

a) Move the paragraph that starts "This hook should only..." up to the top (after the first line), and reword it to explain what the hook should be used to do, like maybe adding a sentence at first that doesn't have the word "only" in it, or taking the "only" out, so the proper rather than the improper use is emphasized. I.e., make this paragraph into a summary of what the hook is and isn't used for.

b) Move the paragraph about when the hook is called to the end, and fix the wrong function in it [field_attach_node_revision() should be field_attach_load_revision().]

c) Move the information that is specific to specific parameters to the @param section for that parameter.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.59 KB

Here ya go rainbow killer ;-) Appreciate your work, @jhodgdon!

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me! I'll leave it at RTBC for a bit before committing it to see if any of the other participants here would like to comment. Thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Well, I think this has been here long enough... but unfortunately the patch needs a reroll as it does not apply to D8 any more. :(

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.83 KB

Re-roll.

jhodgdon’s picture

Status: Needs review » Needs work

This bit didn't make it into the re-rolled patch:

- * field_attach_node_revision() or field_attach_load() is called, then
+ * field_attach_load_revision() or field_attach_load() is called, then

Other than that, looks good...

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
763 bytes
3.04 KB

Sorry about that.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, I'll get this committed shortly!

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x -- thanks again! The patch doesn't apply to 7.x so it needs a port.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
3 KB

Backported.

jhodgdon’s picture

Status: Needs review » Fixed

Thanks! Committed to 7.x.

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