Follow-up to #2513266: Twig templates can call delete() on entities and other objects

Problem/Motivation

#2513266: Twig templates can call delete() on entities and other objects removed some docs about node objects from node.html.twig, but a common use case is arguably to do {{ node.createdtime|format_date('my_great_date_format') }} and this is obscure without those docs. Similar arguments probably apply to the other method docs that were removed.

- * - node: Full node entity.
- *   - id: The node ID.
- *   - bundle: The type of the node, for example, "page" or "article".
- *   - authorid: The user ID of the node author.
- *   - createdtime: Time the node was published formatted in Unix timestamp.
- *   - changedtime: Time the node was changed formatted in Unix timestamp.
+ * - node: The node entity with limited access to object properties and methods.
+     Only "getter" methods (method names starting with "get", "has", or "is")
+     and a few common methods such as "id" and "label" are available. Calling
+     other methods (such as node.delete) will result in an exception.

Proposed resolution

Add back some or all of these docs.

Remaining tasks

Patch
Review

User interface changes

n/a

API changes

n/a

Data model changes

None.

CommentFileSizeAuthor
#44 interdiff-2610344-40-44.txt3.65 KBdimaro
#44 re_add_some-2610344-44.patch5.64 KBdimaro
#40 2610344-36-40.interdiff.txt5.88 KBmikeker
#40 2610344-40-docs-in-node-template.patch6.04 KBmikeker
#36 interdiff-2610344-34-36.txt4.07 KBdimaro
#36 re_add_some-2610344-36.patch5.82 KBdimaro
#34 interdiff-2610344-31-34.txt3.14 KBdimaro
#34 re_add_some-2610344-34.patch5.82 KBdimaro
#31 interdiff-2610344-25-31.txt5.02 KBmohit_aghera
#31 re_add_some-2610344-31.patch5.84 KBmohit_aghera
#25 re_add_some-2610344-25.patch5.87 KBdimaro
#22 re_add_some-2610344-22.patch5.86 KBdimaro
#20 2610344-20-docs-in-node-template.patch5.81 KBmikeker
#20 2610344-17-20.interdiff.txt13.28 KBmikeker
#17 2610344-17-docs-in-node-template.patch12.51 KBmikeker
#17 2610344-14-17.interdiff.txt3.74 KBmikeker
#14 2610344-14-docs-in-node-template.patch11.49 KBmikeker
#14 2610344-11-14.interdiff.txt5.68 KBmikeker
#11 2610344-11-docs-in-node.html_.twig_.patch12.63 KBmikeker
#11 2610344-9-11.interdiff.txt10.98 KBmikeker
#9 re_add_some-2610344-9.patch5.48 KBandriyun
#4 2610344-4.patch3.31 KBanil280988
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cottser created an issue. See original summary.

star-szr’s picture

Issue tags: +Novice
joelpittet’s picture

Issue tags: +Documentation
anil280988’s picture

Status: Active » Needs review
FileSize
3.31 KB

Created the patch.

Status: Needs review » Needs work

The last submitted patch, 4: 2610344-4.patch, failed testing.

mikeker’s picture

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. :)

anil280988’s picture

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

joelpittet’s picture

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.

andriyun’s picture

Assigned: Unassigned » andriyun
Status: Needs work » Needs review
FileSize
5.48 KB

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

joelpittet’s picture

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?

mikeker’s picture

@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).

joelpittet’s picture

Assigned: andriyun » star-szr

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

andriyun’s picture

@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?

mikeker’s picture

@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?

joelpittet’s picture

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

andriyun’s picture

@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?

mikeker’s picture

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...

star-szr’s picture

This is on my list to look at, thanks everyone :)

star-szr’s picture

Assigned: star-szr » Unassigned
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -rc eligible

Thanks everyone for keeping this one going :)

  1. +++ b/core/modules/node/templates/node.html.twig
    @@ -5,12 +5,19 @@
    + *   For example,
    + *     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
    

    I think to mesh with our doc standards this should probably be a list like https://www.drupal.org/node/1354#lists and end each item with a period per https://www.drupal.org/node/1354#drupal.

  2. +++ b/core/modules/node/templates/node.html.twig
    @@ -5,12 +5,19 @@
    + *   See the API documentation for a full list of public properties and methods
    + *   for the node object.
    

    Nice. Can we make this link to the relevant API docs? Even if it might end up slightly ugly in the template it would be nice for api.d.o I think.

    And/or we could add another @see.

  3. +++ b/core/modules/node/templates/node.html.twig
    @@ -5,12 +5,19 @@
    - * - content: All node items. Use {{ content }} to print them all,
    - *   or print a subset such as {{ content.field_example }}. Use
    + * - content: All node items. Use {{ content }} to print them all, or print a
    + *   subset such as {{ content.field_example }}. Use
    
    @@ -36,12 +43,12 @@
    - * - title_attributes: Same as attributes, except applied to the main title
    + * - title_attributes: Same as attributes, except applied to the main title tag
    + *   that appears in the template.
    + * - content_attributes: Same as attributes, except applied to the main content
    ...
    - * - content_attributes: Same as attributes, except applied to the main
    - *   content tag that appears in the template.
    - * - author_attributes: Same as attributes, except applied to the author of
    - *   the node tag that appears in the template.
    + * - author_attributes: Same as attributes, except applied to the author of the
    + *   node tag that appears in the template.
    
    @@ -53,8 +60,8 @@
    - * - is_admin: Flag for admin user status. Will be true when the current user
    - *   is an administrator.
    + * - is_admin: Flag for admin user status. Will be true when the current user is
    + *   an administrator.
    

    I know these are all wrapping early but these changes are out of scope. I'd rather these happen in a separate issue (minor task I think) and keep this one focused and easier to review, reroll, etc. and ultimately get in quicker.

mikeker’s picture

Status: Needs work » Needs review
FileSize
13.28 KB
5.81 KB

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! :)

joelpittet’s picture

@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...

dimaro’s picture

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"?

mikeker’s picture

Status: Needs review » Needs work

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?

mikeker’s picture

+++ 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!

dimaro’s picture

Status: Needs work » Needs review
FileSize
5.87 KB

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?

mikeker’s picture

Status: Needs review » Needs work

@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!

dimaro’s picture

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

Thanks!

joelpittet’s picture

@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.

dimaro’s picture

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?

mikeker’s picture

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.

mohit_aghera’s picture

Status: Needs work » Needs review
FileSize
5.84 KB
5.02 KB

Adding changes mentioned by @mikeker and @joelpittet

dimaro’s picture

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
mikeker’s picture

Status: Needs review » Needs work

@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

dimaro’s picture

Status: Needs work » Needs review
FileSize
5.82 KB
3.14 KB

Update to solve this little change.

mikeker’s picture

Status: Needs review » Needs work

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.

dimaro’s picture

Status: Needs work » Needs review
FileSize
5.82 KB
4.07 KB

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

mikeker’s picture

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.

andriyun’s picture

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

star-szr’s picture

Status: Needs review » Needs work

Thanks everyone :) some thoughts:

  1. +++ b/core/modules/node/templates/node.html.twig
    @@ -5,9 +5,17 @@
    + *   Only "getter" methods (method names starting with "get", "has", or "is")
    

    I could be wrong but I don't think has* or is* are considered getters :) maybe just ditch the getters part and say "Only method names starting with "get", "has", or "is" and a few common…"?

  2. +++ b/core/modules/node/templates/node.html.twig
    @@ -5,9 +5,17 @@
    + *   - node.hasField('field_example') returns TRUE if the node includes
    + *     field_example.
    

    Can we somehow make this clearer that it's about whether the node's bundle has the field defined, not whether the specific node has content in that field? That might be confusing to people who are expecting it to do something different.

  3. +++ b/core/modules/node/templates/node.html.twig
    @@ -5,9 +5,17 @@
    + *   Calling other methods (such as node.delete) will result in an exception.
    

    For consistency maybe use node.delete() here?

mikeker’s picture

Status: Needs work » Needs review
FileSize
6.04 KB
5.88 KB

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.

dimaro’s picture

This issue need some work?
RTBC maybe?

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

RTBC I'd say. Thanks.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Nitpick time, just one small thing then back to RTBC land :)

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

This has been bugging me a bit, in that the @link here is a bit ugly and perhaps not as effective as a reference to the class, which some IDEs would (might?) turn into a link automatically, and api.drupal.org would link automatically. I also searched through our code and only found one instance of @link linking to api.drupal.org in 8.0.x:

* - @link https://api.drupal.org/api/drupal/groups/8 All topics @endlink

An example of this can be seen in the container.html.twig docs: https://api.drupal.org/api/drupal/core%21modules%21system%21templates%21...

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

dimaro’s picture

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?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dimaro’s picture

Update #44 to run against 8.1.x

#44 RTBC maybe?

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Sorry for the delay (and thanks for the bump), I think this is good now. Thanks @dimaro!

dimaro’s picture

Issue tags: +DrupalCampES

Tagging this issue.

  • Cottser committed 526501d on 8.2.x
    Issue #2610344 by mikeker, dimaro, mohit_aghera, andriyun, anil280988,...

  • Cottser committed 6ef1402 on 8.1.x
    Issue #2610344 by mikeker, dimaro, mohit_aghera, andriyun, anil280988,...
star-szr’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 526501d to 8.2.x and 6ef1402 to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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