Re-add some documentation about what you can get from the node object in node.html.twig

Created the patch.

Has anyone checked that node.authorid works? I don't believe there is a Node::getAuthorId() method. For what it's worth, the node.html.twig (x3 for Bartik, Classy, and core) is the only template that had docs lines removed in the original issue.

Do we want to properly document everything that's available in the default whitelist? That'll be a much longer list and I'm concerned that it will get out of sync with what's available from the Node object. Is there a better way to present that? Because I agree that we want to give themers the info they need.

Also, I think the patch in #4 is going the wrong way -- removing the lines that have already been removed. :)

Hi Mikeker,
Could you point me out in right direction.

This needs one in Stable now too.

And @mikeker the answer is no node.authorId doesn't exist. I think somewhere it got converted node.ownerId

Don't want to document everything, only the variables that are used in the template. If there are more say so.

Restored description for available properties for node variable.
Changed authorid to ownerId

Thank you @andriyun!

+++ b/core/themes/classy/templates/content/node.html.twig
@@ -4,10 +4,15 @@
+ *   - ownerId: The user ID of the node author.
+ *   - createdtime: Time the node was published formatted in Unix timestamp.

I'm kind of considering(if not already documented) we should use the CamelCase, both for some consistency with the methods and because it's hard to tell if we should or shouldn't. @mikeker you had some thoughts on this?

@joelpittet, I do, indeed, have some opinions on this! :)

However, our coding standards are confusing in this regard. The Drupal standards for Twig says to follow the Twig's standards. Twig says to use lower-cased, underscored-separated variable names.

That seems pretty clear except that Twig's magic variable resolution allows us to use node.foobarbaz or node.fooBarBAZ to call $node->getFooBarBaz() and Drupal says that method names are camelCased.

Personally, I would like to see us match Twig variables to the method/property they are referencing. I think it will help reduce confusion and make for easier lookup in the API docs.

(This patch also cleans up some 80-char limit issues -- sorry, couldn't resist).

@mikeker I agree completely let's get @Cottser to weigh in and make that written hard in pencil!

@mikeker

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -4,13 +4,18 @@
   - * - node: The node entity with limited access to object properties and methods.
   

   It's ok what this line is deleted?

2. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -4,13 +4,18 @@
   - * - node: The node entity with limited access to object properties and methods.
   

   It's ok what this line is deleted?

3. +++ b/core/themes/classy/templates/content/node.html.twig
   @@ -4,13 +4,18 @@
   - * - node: The node entity with limited access to object properties and methods.
   

   It's ok what this line is deleted?

4. +++ b/core/themes/stable/templates/content/node.html.twig
   @@ -4,13 +4,18 @@
   - * - node: The node entity with limited access to object properties and methods.
   

   It's ok what this line is deleted?

@andriyun, yeah, sorry about that. The removal showed up in #9 and I copy/pasted it all over the place in this last patch. Fixed in the attached patch.

Slightly different approach in this patch: rather than pointing out a handful of methods such as createdTime I think we should point people to the API docs for the node object. As @joelpittet said in #8, we don't want to try and document everything. Thoughts?

I'm a fan of this approach, @andriyun does that work for you?
Leaving this assigned to hawkeye for feedback.

@mikeker Nice variant too. Thank you!
But in issue reason is restore this

... valuable docs

as Cottser said in followup comment https://www.drupal.org/node/2513266#comment-10538356

I propose restore docs about very useful properties of node object and add mention about API reference to other properties
It's will be more clear.

What you think about it?

I suppose that might have been a bit sparse. I'd added a few examples to the docblock that show using an is/has/get function. Or were you concerned specifically about the changedTime and ownerId items?

I would love to hear @Cottser's opinion on the coding style of these examples. In this patch, I'm going with the same text and casing as the function being called on the node object: getCreatedTime(). But any of

node.getcreatedtime
node.getcreatedtime()
node.createdtime
node.createdTime
node.CreatedTime

(among others) would work. Again, for clarity, I like being as specific as possible: including the "get", indicating that this is a function call with parens, using the same casing as the underlying method. But I've gotten push-back in other issues for this being too verbose...

re: #19

#1: Done. Thanks for the pointer to the doc standards!
#2: I've added the link to the API docs and, yup, it looks ugly! :) One concern is that the api doc URLs change as the referenced file gets moved around, eg: PSR-0 and then PSR-4 standards. Hopefully URLs will stay put now that we've got 8.0 out the door. But the autoloader makes this a moving target. Regardless, it's there for now and we can update it if it changes.
#3: Man, I thought with @Cottser's new superpowers we would be able to sneak this in! (just kidding! :)

@andriyun want to take a stab at these and have another look?

Thanks for the changes @mikeker and @Cottser for the review.

1. +++ b/core/themes/classy/templates/content/node.html.twig
   @@ -5,9 +5,17 @@
   + *       field_example
   

   This can be pulled back 2 spaces and end in a period.

2. +++ b/core/themes/classy/templates/content/node.html.twig
   @@ -5,9 +5,17 @@
   + *   - node.isPublished() will return whether the node is published or not
   + *   Calling other methods (such as node.delete) will result in an exception.
   

   Calling should be lower case 'c'

3. +++ b/core/themes/classy/templates/content/node.html.twig
   @@ -5,9 +5,17 @@
   + *   (https://api.drupal.org/api/drupal/core%21modules%21node%21src%21Entity%21Node.php/class/Node/8)
   

   I'm not sure if the url needs parenthesis and shouldn't it reference the NodeInterface maybe, just a thought since we have those...

   https://api.drupal.org/api/drupal/core!modules!node!src!NodeInterface.ph...

I have tried to correct the points listed above, I have a doubt in point 3.

Would it be correct indicate the reference to the Node class with the prefix "@link" and suffix "@endlink"?

Thanks @dimaro for the updates!

@joelpittet, re: #21:

shouldn't it reference the NodeInterface maybe

I thought about that. In this case they are virtually identical. I think only getCurrentUserId is added to Node beyond what the interface specifies. (And, honestly, I'm not really sure why it's there other than as a shortcut -- it doesn't have much to do with nodes). But the idea is that the Node object (or any parent class) could provide so additional functionality beyond the interface spec and it would be nice to reference that.

Do we usually point docs to the class interface instead of the class?

+++ b/core/modules/node/templates/node.html.twig
@@ -5,9 +5,17 @@
+ *   See the @link https://api.drupal.org/api/drupal/core%21modules%21node%21src%21Entity%21Node.php/class/Node/8
+ *   @endlink API documentation.

Sorry, this was the reason I set it to NW, above, but forgot to include... The format for links is @link http://example.com text to be linked @endlink. I think it would be cleaner to have "API documentation" be the "text to be linked" part. Otherwise the docs parser uses the URL as the text to be linked which is quite long in this case!

Thanks @mikeker for your review.

I have updated the patch to replace the URL and indicate the reference to NodeInterface.
I have also introduced the text "API documentation" as "text to be linked"

Would be fine these changes?

@dimaro, Thanks for the updates. My apologies for doing an incomplete review of the earlier patch -- I was just commenting on one aspect of it but should've read through the rest as well.

A few other nitpicks:

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -5,9 +5,17 @@
   + *   calling other methods (such as node.delete) will result in an exception.
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -5,9 +5,17 @@
   + *   calling other methods (such as node.delete) will result in an exception.
   
   +++ b/core/themes/classy/templates/content/node.html.twig
   @@ -5,9 +5,17 @@
   + *   calling other methods (such as node.delete) will result in an exception.
   
   +++ b/core/themes/stable/templates/content/node.html.twig
   @@ -5,9 +5,17 @@
   + *   calling other methods (such as node.delete) will result in an exception.
   

   I realize @joelpittet said earlier that these should start with a lower-case 'c', but it's actually a new sentence and should start with an upper-case 'C'.

2. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -5,9 +5,17 @@
   + *   - node.getCreatedTime() will return the node creation timestamp
   + *   - node.hasField('field_example') returns TRUE if the node includes
   + *     field_example
   + *   - node.isPublished() will return whether the node is published or not
   
   +++ b/core/themes/classy/templates/content/node.html.twig
   @@ -5,9 +5,17 @@
   + *   - node.getCreatedTime() will return the node creation timestamp
   + *   - node.hasField('field_example') returns TRUE if the node includes
   + *     field_example
   + *   - node.isPublished() will return whether the node is published or not
   
   +++ b/core/themes/stable/templates/content/node.html.twig
   @@ -5,9 +5,17 @@
   + *   - node.getCreatedTime() will return the node creation timestamp
   + *   - node.hasField('field_example') returns TRUE if the node includes
   + *     field_example
   + *   - node.isPublished() will return whether the node is published or not
   

   Needs periods at the end of each list item. Details here: https://www.drupal.org/coding-standards/docs#lists.

Setting to NW for the above items, but it might be best to wait until we've heard from @joelpittet about whether to link to the NodeInterface or Node object. I'll ping him on IRC. Also, when updating patches, including an interdiff makes it a lots easier to see the changes from one version to the next. Thanks!

My apologies for not attach the interdiff.
I'm agree to wait until @joelpittet can review this issue.

Thanks!

@mikeker @dimaro yeah whoops, I said lower case c because I thought it was part of the sentence above, I read the second one and it was missing a period above line and all subsequent ones, the first one had the period.

I'm fine with whichever you decide is best NodeInterface or Node. They both give you helpful information and very similar like @mikeker mentioned. So maybe keep it Node.

Thanks @joelpittet for your review, I will update the patch to include these periods at the end of each item.
As for the link, the latest patch refers to NodeInterface, we should change it back to Node?

If @joelpittet doesn't care, I'd prefer changing the link it back to Node since that will include any future additions to the Node object (or it's parents) that may not be expressed in the NodeInterface.

@dimaro, thank you for your work on this and my apologies for the back-and-forth on some of the requested changes.

Adding changes mentioned by @mikeker and @joelpittet

I think that the link should be on one line and in any case it should be continued up to 80 characters.
@joelpittet @mikeker what do you think about this?

+++ b/core/modules/node/templates/node.html.twig
@@ -5,9 +5,17 @@
+ *   See the @link https://api.drupal.org/api/drupal/core!modules!node!src!
+ Entity!Node.php/class/Node/8
+ *   API documentation @endlink for a full list of public properties and methods

@link ... @endlink needs to be all on one line, even if that line goes over 80 chars. If it can fit with the text of the line before or after the link without going over 80 chars, then it should be grouped with that text. Otherwise it should be on it's own line. Something like

See the
@link really_long_url API documentation @endlink
for a full list... 

Details here: https://www.drupal.org/coding-standards/docs#link

Update to solve this little change.

Wow! That was quick!

+++ b/core/modules/node/templates/node.html.twig
@@ -5,9 +5,17 @@
+ *   See the @link https://api.drupal.org/api/drupal/core!modules!node!src!Entity!Node.php/class/Node/8
+ *   API documentation @endlink for a full list of public properties and methods

+++ b/core/themes/bartik/templates/node.html.twig
@@ -5,9 +5,17 @@
+ *   See the @link https://api.drupal.org/api/drupal/core!modules!node!src!Entity!Node.php/class/Node/8
+ *   API documentation @endlink for a full list of public properties and methods

+++ b/core/themes/classy/templates/content/node.html.twig
@@ -5,9 +5,17 @@
+ *   See the @link https://api.drupal.org/api/drupal/core!modules!node!src!Entity!Node.php/class/Node/8
+ *   API documentation @endlink for a full list of public properties and methods

+++ b/core/themes/stable/templates/content/node.html.twig
@@ -5,9 +5,17 @@
+ *   See the @link https://api.drupal.org/api/drupal/core!modules!node!src!Entity!Node.php/class/Node/8
+ *   API documentation @endlink for a full list of public properties and methods

The @link ... @endlink needs to be on its own line in this situation. See #33.

I would say very fast, and caused my mistake, sorry.
Update patch.
@mikeker Thank you for your review.

Looks great! Thanks, @dimaro, for moving this forward. I can't RTBC it since I worked on the patch, but I'll ping Joel or Scott to take a look at it.

Good job @dimaro. Looks great for me too!
+1 to RTBC

I could be wrong but I don't think has* or is* are considered getters :)

Ha! Good point! I don't think calling them "idempotent" would clarify things any so I changed the line to what you suggested.

Also updated the text to address #2 and #3.

This issue need some work?
RTBC maybe?

So I would propose that we replace the @link…@endlink stuff with "See \Drupal\node\Entity\Node for a full list…".

Fixed.

Needs some more work?

Update #44 to run against 8.1.x

#44 RTBC maybe?

Tagging this issue.