Clean up cruft in the Action documentation

Since documentation oriented patches tend to be assigned to @jhodgdon for commit, and Jennifer prefers to handle patches with just documentation separately from those that include type hinting, I have opened #1802070: Add missing type hinting to Action module docblocks to handle the type hinting aspect of the above comments.

Here is an untested patch addressing the issue summary (except for type hinting) as well as a full documentation review of this new module. Comments on this patch are welcome.

For the record, if you're adding completely new param/return docs (that were previously missing), since that also requires reading the function to give them a thorough review, I have no problem with adding the types to them at the same time in the same patch.

Thanks! I gave this a quick look... A few things are not conforming to our standards:

a) Form constructor functions - the standard is that we don't document $form, $form_state or @return for these. [action_admin_manage_form() has @return and shouldn't]

b) It's not necessary to leave a blank line between @see and @ingroup at the end of a doc block.

c) Class documentation first lines should have the same verb tense as functions [see LoopTest]

And I have a question about this:
d)

* @param object $entity
- *   An optional node entity, which will be added as $context['node'] if
- *   provided.
+ *   A node entity, which will be added as $context['node'] if that is empty.

This actually changes the meaning of the @param documentation, rather than being just a clean-up. Was that intentional? I haven't looked at the function body to verify... [change was made in two places]

Also, let's wait on continuing with this issue, because it's being worked on elsewhere:
#1431658: Clean up API docs for system module tests, A-D, including subdirectories

Looks like #1431658: Clean up API docs for system module tests, A-D, including subdirectories still needs considerable work. Hence, returning this to active since it appears that there are only a few open items for this issue.

This patch will conflict with that one, and much of what is in this patch had already been done on that other issue. I'd like to keep this one postponed until that one gets finished, if you don't mind... or else make sure this patch doesn't cover the same stuff?

I am not sure why your request makes sense @jhodgdon. The focus of this issue is making the new D8 module conformant with D8 documentation standards.

When we have issues with large patches, they are a pain to reroll when things get committed that conflict with them. That other issue was started before this code was split off into a new module, so it has been making changes to what is now in this new module, and has been following the code around as it moves around in the code base.

So, I'd like to get that patch finalized and committed before we make conflicting changes that will necessitate yet another reroll to that large patch. OK?

Quite frankly, if anyone wants to make the new Action module conformant with D8 documentation standards, they are welcome to pursue this issue further.

In the original issue, @sun and @catch stated that adding documentation fixes would be too difficult to review the move from an include file to a new Action module. Further, @sun decided that some documentation fixes were OK for the Ban module move, but for some unclear reason, such documentation changes would be inappropriate to the new Action module.

To address concerns about documentation, a follow up issue was created: This one!! Of course, this follow up issue has not received any attention like the original move issue. One might even reasonably conclude that documentation standards matter little more than lip service.

Nonetheless, after diligently creating a patch in #4, this issue is getting postponed because of something supposedly being proposed in #1431658: Clean up API docs for system module tests, A-D, including subdirectories.

I appreciate all of those involved with their concerns for proper procedures. However, the net result of this whole experience is my firm belief questioning why any core patch needs to conform with documentation standards. Of course, I might somehow be enlightened by this incredible process.

And just to be clear, both @sun and @catch, who were behind the decoupling of actions.inc to the new Action module, at least to my knowledge, have not bothered to confirm that the documentation is conformant (at least to some extent with D8 doc standards).

I am completely stumped about why the move was OK, but documentation fixes get first brushed off, first to a follow up issue, and then postponed.

My take away from the last few weeks is the original code review in #1008166-51: Actions should be a module has not had any effect and whatever was in the patch is simply OK.

I now know that such contributions to the Drupal review process are unwelcome and likely ignored. I have little interest in this ignore and postpone process.

I really don't think there are any grand conspiracies here.

1) Type-hinting is not a formal part of the documentation gate, and therefore is not currently something we hold back RTBC patches on. There is an issue to discuss this at #1792992: Are typed @param / @return part of the documentation gate? but consensus is not yet reached, so at the moment the existing gate (which does not include type-hinting) still stands.

2) When there are two patches that conflict with each other, the normal rule is "first to RTBC wins." However, it can be a good idea to postpone one on another for a variety of reasons: patch A is much further along and closer to get committed than patch B, patch A needs much more thorough reviews than patch B and so is much further off from RTBC, patch B is *much* harder to re-roll than patch A (esp. if it fixes a major/critical issue), etc.

3) Frankly, people are under absolutely no obligation to work on what you want them to work on. And they are even less inclined when your words and attitude imply they are lazy, belligerent, negligent, incompetent, or some combination of the above. If you find people aren't responding well to your requests for patch reviews, stop yourself and ask if the way you are conducting yourself in the issue queue might have something to do with that.

I don't have a horse in this race. Both patches deserve to be reviewed/committed, and no one is disputing that. But it looks like #1431658-26: Clean up API docs for system module tests, A-D, including subdirectories is a 127K patch, which focuses purely on API documentation changes and thus is much easier to review, much harder to re-roll, as well as less risky to commit. The patch here is only 27K and includes a wide range of changes, including docs fixes, coding standards fixes, etc. This is probably why it's being postponed in favour of the other. And rather than assuming sun and catch are liars who don't follow through on their commitments, how about assume they are busy frantically trying to get features into Drupal 8 while there's still time, and that patches like this will receive much more active review attention once we're at the point in the cycle that's designed specifically for this type of clean-up. Better yet, how about ask them how you can help them in their mission? That'll give them much more incentive to help you in return.

Lars, I think you do really thorough, meticulous work with documentation, and that's a huge asset to the team. But the way you treat other people is tearing the team apart. Please read http://drupal.org/dcoc and take the words to heart. "Come for the software, stay for the community" isn't just something we put on our website to feel good about ourselves. It's critical to a well-functioning core development team.

@webchick Thanks for your thoughts. If and when this issue gets committed, I will respect that the event has occurred.

In the meantime, I will keep my thoughts to myself about the last paragraph of #14. I very much disagree, but I do not want to disrupt the supposed "core development" team.

That other patch got committed, so this one can be opened again. Setting status as per last patch review; also it will need a small reroll as the other patch conflicted with it in a couple of spots.

Thanks @jhodgdon. Someone else can drive this one home.

Apparently those involved with creating the new Action module do not care enough about documentation standards. Or if they do, they have yet to show up in this follow up issue.

The people making the Action module just moved the existing code from actions.inc to action.module. It was not part of their responsibility to *also* clean up the API docs in the process. That would be scope creep.

Re-roll of #4. Still needs work per the comments in #6.

Setting status.

Here we go, re-rolled and additional concerns (I think) addressed.

Can we take out all of the ridiculous "Defaults to ..." statements in this patch, which provide no information not already found in the function signatures?

Other than that, this patch looks fine... there are a few other things that could be improved (like "boolean" that should be "Boolean" in text, and in the LoopTest.php hunk at the end, describing what action ID the class is storing -- "an action ID" doesn't really tell me much about what that class member variable is being used for?)... but nothing I would probably hold up committing on.

Can we take out all of the ridiculous "Defaults to ..." statements in this patch, which provide no information not already found in the function signatures?

Uh, well, once upon a time when I was a novice writing documentation patches, the standard was to end the parameter description with what default value was provided and why. :) Has the standard changed in the past 18 months?

If there's a reason to include the information, sure. But this has never been the standard that I'm aware of in general, and I think there are usually better ways to give the information than a "Defaults to NULL"... In this patch, I can't see anywhere it is really adding any information. Here are some examples:

+ *   (optional) The object that the action will act on: a node, user, or comment
+ *   object. Defaults to NULL.

Already says it's optional, so obviously you are allowed not to provide it, and it hardly matters what the default value is (that is something the code but not the caller would care about; in that case, look at the function sig along with the code).

+ *   (optional) Associative array containing extra information about what
+ *   triggered the action call, with $context['hook'] giving the name of the
+ *   hook that resulted in this call to actions_do(). Additional parameters
+ *   will be used as the data for token replacement. Defaults to NULL.

Ditto.

+ *   (optional) Passed along to the callback. Defaults to NULL.
 

Ditto.

+ *   (optional) A boolean indicating whether to reset the action info static
+ *   cache. Defaults to FALSE.

Would be a lot better to write "(optional) If set to TRUE, reset the static cache of action information."

+ *   (optional) The ID of this action. If omitted, a new action is created.
+ *   Defaults to NULL.

In this case, the text already says what happens if it is omitted, so the "Defaults to NULL" tells us nothing new and just wastes space.

etc.

Hm. I don't personally find those very offensive. When I'm reading docs, I'm reading docs. When I'm reading code, I'm reading code. Sometimes the two might be separated by a very large gulf of @return and @see and other things, or, on a site like http://api.drupal.org/, separated by totally different formatting, making moving my eyeball back and forth to gather up more information a bit jarring.

However, it is definitely duplicate information and can easily fall out of sync in said "gulfy" situations if people aren't careful. Is it helpful enough to counter-act the potential confusion if this happens? Couldn't say, and I don't really feel overly strong about it.

We could probably settle this by grepping and seeing what we do more often than not, and just continuing to do that. :)

Hm. I don't personally find those very offensive. When I'm reading docs, I'm reading docs. When I'm reading code, I'm reading code. Sometimes the two might be separated by a very large gulf of @return and @see and other things, or, on a site like http://api.drupal.org/, separated by totally different formatting, making moving my eyeball back and forth to gather up more information a bit jarring.

Yeah, I'd agree with this too, actually.

I could see the argument either way (duplicate content/falling out of sync vs readability). The pragmatic side of me says to just use the function's signature. Reduces work, and could possibly be parsed in a more useable way via a.d.o (being completely naïve to the limitations of the doc parsing)? I'm fine either way, however =)

My understanding was that the docblock should describe what a function/method does without needing to dig into/read the code at all. Hence, what the default value of a parameter is needed to be part of the documentation. This may have changed though in the last six months or so since I last worked on documentation issues.

But "Defaults to NULL" adds exactly zero information in these cases. If you say (optional), it means the parameter is optional, and presumably if you don't supply it that is OK. Adding "Defaults to NULL" does not tell us anything at all, does it?

I am completely in agreement that in the case where the default value is more complicated or has some meaning beyond "it's not provided" when the @param says (optional), we should tell what the default value is. But in the case of "Defaults to NULL" it is IMO a waste of space/time, a burden to doc writers to set this as a precedent or standard, and a maintenance burden as well. Let's not make a habit of adding it to otherwise perfectly clear documentation.

I also don't agree that "reading the signature" is the same as "reading the code". On api.drupal.org, the function signature is right up there, and in IDEs it will be shown to you as well.

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!