Problem/Motivation

Original Report:

+++ b/core/modules/comment/comment.module
@@ -700,6 +700,7 @@ function template_preprocess_comment(&$variables) {
+  $variables['author_id'] = $comment->getOwnerId();

+++ b/core/modules/comment/templates/comment.html.twig
@@ -64,7 +64,16 @@
+    not author_id ? 'by-anonymous',
This perpetuates the "user ID zero is the anonymous user" check which IMHO makes no sense.

This should've used $comment->getOwner->isAnonymous().

Proposed resolution

Remaining tasks

User interface changes

API changes

Follow-up to #2329783: Move comment classes from preprocess to templates

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Issue tags: +Novice, +php-novice
Tom Verhaeghe’s picture

Assigned: Unassigned » Tom Verhaeghe
Tom Verhaeghe’s picture

Status: Active » Needs review
FileSize
1.73 KB

Added new variable 'author_anonymous' and updated twig templates to use that in both the comments module and bartik.

Wim Leers’s picture

Wouldn't author_is_anonymous be better?

Tom Verhaeghe’s picture

Conventionally that would be better, I also didn't update the twig documentation. I'll fix that ...

Tom Verhaeghe’s picture

FileSize
2.55 KB
2.63 KB
Wim Leers’s picture

Title: Comments to check '$comment->getOwner->isAnonymous().' instead of assuming anonymous is ID 0 » Comments to check '$comment->getOwner->isAnonymous()' instead of assuming anonymous is ID 0
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community

Thanks! Looks great.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/comment.module
@@ -699,6 +699,7 @@ function template_preprocess_comment(&$variables) {
+  $variables['author_is_anonymous'] = $comment->getOwner()->isAnonymous();

+++ b/core/modules/comment/templates/comment.html.twig
@@ -70,7 +72,7 @@
-    not author_id ? 'by-anonymous',
+    author_is_anonymous ? 'by-anonymous',

Can we not use comment.getOwner().isAnonymous() in twig instead of a new variable?

joelpittet’s picture

@alexpott Yes we can. We can also use: comment.owner.anonymous ? 'by-anonymous' or any combo that reads the best.

@Tom Verhaeghe what are your thoughts here, any preference?

Wim Leers’s picture

#8: oh, I didn't know that was possible, that's even better :)

joelpittet’s picture

From our slides:)
http://drupaltwig.github.io/ThemeSystemBadCamp2014/#/5

The more people that know this magic/syntax sugar the better

Wim Leers’s picture

#11: Great, thanks! :) That was very helpful :)

Tom Verhaeghe’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
2.59 KB

Changed the way to check if the author is anonymous.

PS: Is it possible that calling methods on objects is done this way comment.getOwner().isAnonymous() instead of comment->getOwner()->isAnonymous() as shown in the slides?

Wim Leers’s picture

Assigned: Tom Verhaeghe » joelpittet

Assigning to Joël for review.

joelpittet’s picture

@Tom Verhaeghe sorry our slides could be clearer.

{{ comment.owner.anonymous }} will get converted to PHP eventually. When it does, each "attribute" (the thing after the dot, Twig's language for what we usually refer to as a member variable or property) will be converted to PHP in the following ways:

// Array key.
$comment['owner']['anonymous'];
// Object property.
$comment->owner->anonymous;
// Also works for magic get (provided you implement magic isset).
$comment->__isset('owner')->isset('anonymous'); && $comment->__get('owner')->__get('anonymous');
// Object method.
$comment->owner()->anonymous();
// Object get method convention.
$comment->getOwner()->getAnonymous();
// Object is method convention.
$comment->isOwner()->isAnonymous();
// Method doesn't exist/dynamic method.
$comment->__call('owner')->__call('anonymous');

In that order of checking until it finds the correct PHP 'thing'.

In this specific case it will do the following (theory and rough equivalent):

if ($comment->__isset('owner')) {
  $owner = $comment->__get('owner');
  return $owner->isAnonymous();
}

Here's an example of all the things that work:

mkdir magic && cd magic
composer require "twig/twig:~1.0"
wget https://gist.githubusercontent.com/joelpittet/c5a20ddd1621e4d07e88/raw/10181c75b830510fa6d4f0f73e2ec77dd32fd34a/index.php

Then visit http://localhost/magic
https://gist.github.com/c5a20ddd1621e4d07e88

So I personally recommend the calls without the method calls, though #13 will technically work and is fine:)

joelpittet’s picture

Assigned: joelpittet » Unassigned

Long story short, #13 is good as-is and should work beautifully though I'd prefer the slightly simpler syntax.

Tom Verhaeghe’s picture

FileSize
1.14 KB
1.1 KB

Okay, thanks for explaining. It's clear to me now :-)

I changed the syntax in this patch.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

RTBC per #16.

(That's quite a bit of magic though.)

alexpott’s picture

Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed

Reclassifying as a bug since

"user ID zero is the anonymous user" check which IMHO makes no sense

Sounds like one.

This issue is a minor bug fix, and doesn't include any disruptive changes, and is in the unfrozen category (templates) so it is allowed per https://www.drupal.org/core/beta-changes. Committed 010cfbb and pushed to 8.0.x. Thanks!

  • alexpott committed 010cfbb on 8.0.x
    Issue #2372909 by Tom Verhaeghe: Comments to check '$comment->getOwner->...

Status: Fixed » Closed (fixed)

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