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.
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff-2610344-40-44.txt | 3.65 KB | dimaro |
#44 | re_add_some-2610344-44.patch | 5.64 KB | dimaro |
#40 | 2610344-36-40.interdiff.txt | 5.88 KB | mikeker |
#40 | 2610344-40-docs-in-node-template.patch | 6.04 KB | mikeker |
#36 | interdiff-2610344-34-36.txt | 4.07 KB | dimaro |
Comments
Comment #2
star-szrComment #3
joelpittetComment #4
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedCreated the patch.
Comment #6
mikeker CreditAttribution: mikeker as a volunteer commentedHas anyone checked that
node.authorid
works? I don't believe there is aNode::getAuthorId()
method. For what it's worth, thenode.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. :)
Comment #7
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedHi Mikeker,
Could you point me out in right direction.
Comment #8
joelpittetThis needs one in Stable now too.
And @mikeker the answer is no
node.authorId
doesn't exist. I think somewhere it got convertednode.ownerId
Don't want to document everything, only the variables that are used in the template. If there are more say so.
Comment #9
andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commentedRestored description for available properties for node variable.
Changed authorid to ownerId
Comment #10
joelpittetThank you @andriyun!
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?
Comment #11
mikeker CreditAttribution: mikeker as a volunteer commented@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
ornode.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).
Comment #12
joelpittet@mikeker I agree completely let's get @Cottser to weigh in and make that written hard in pencil!
Comment #13
andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented@mikeker
It's ok what this line is deleted?
It's ok what this line is deleted?
It's ok what this line is deleted?
It's ok what this line is deleted?
Comment #14
mikeker CreditAttribution: mikeker as a volunteer commented@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?Comment #15
joelpittetI'm a fan of this approach, @andriyun does that work for you?
Leaving this assigned to hawkeye for feedback.
Comment #16
andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented@mikeker Nice variant too. Thank you!
But in issue reason is restore this
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?
Comment #17
mikeker CreditAttribution: mikeker as a volunteer commentedI 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(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...
Comment #18
star-szrThis is on my list to look at, thanks everyone :)
Comment #19
star-szrThanks everyone for keeping this one going :)
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.
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.
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.
Comment #20
mikeker CreditAttribution: mikeker as a volunteer commentedre: #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! :)
Comment #21
joelpittet@andriyun want to take a stab at these and have another look?
Thanks for the changes @mikeker and @Cottser for the review.
This can be pulled back 2 spaces and end in a period.
Calling should be lower case 'c'
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...
Comment #22
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedI 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"?
Comment #23
mikeker CreditAttribution: mikeker as a volunteer commentedThanks @dimaro for the updates!
@joelpittet, re: #21:
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?
Comment #24
mikeker CreditAttribution: mikeker as a volunteer commentedSorry, 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!Comment #25
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedThanks @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?
Comment #26
mikeker CreditAttribution: mikeker as a volunteer commented@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:
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'.
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!
Comment #27
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedMy apologies for not attach the interdiff.
I'm agree to wait until @joelpittet can review this issue.
Thanks!
Comment #28
joelpittet@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.
Comment #29
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedThanks @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?
Comment #30
mikeker CreditAttribution: mikeker as a volunteer commentedIf @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.
Comment #31
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedAdding changes mentioned by @mikeker and @joelpittet
Comment #32
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedI 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?
Comment #33
mikeker CreditAttribution: mikeker as a volunteer commented@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 likeDetails here: https://www.drupal.org/coding-standards/docs#link
Comment #34
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedUpdate to solve this little change.
Comment #35
mikeker CreditAttribution: mikeker as a volunteer commentedWow! That was quick!
The @link ... @endlink needs to be on its own line in this situation. See #33.
Comment #36
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedI would say very fast, and caused my mistake, sorry.
Update patch.
@mikeker Thank you for your review.
Comment #37
mikeker CreditAttribution: mikeker as a volunteer commentedLooks 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.
Comment #38
andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commentedGood job @dimaro. Looks great for me too!
+1 to RTBC
Comment #39
star-szrThanks everyone :) some thoughts:
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…"?
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.
For consistency maybe use
node.delete()
here?Comment #40
mikeker CreditAttribution: mikeker as a volunteer commentedHa! 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.
Comment #41
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedThis issue need some work?
RTBC maybe?
Comment #42
star-szrRTBC I'd say. Thanks.
Comment #43
star-szrNitpick time, just one small thing then back to RTBC land :)
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…".
Comment #44
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedFixed.
Needs some more work?
Comment #46
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedUpdate #44 to run against 8.1.x
#44 RTBC maybe?
Comment #47
star-szrSorry for the delay (and thanks for the bump), I think this is good now. Thanks @dimaro!
Comment #48
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedTagging this issue.
Comment #51
star-szrCommitted and pushed 526501d to 8.2.x and 6ef1402 to 8.1.x. Thanks!