Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The variable $in_preview is used in the files:
- comment.module (lines 587, 676 and 735)
- CommentDefaultFormatter.php (line 149)
- CommentViewBuilder.php (lines 140 and 171)
- CommentLazyBuilders.php (lines 127, 133 and 140)
- rdf.module (line 431)
This variable is not declared in the entity Comment and that is not correct.
The entity class Node has also a variable $in_preview and that variable is set public for performance reasons.
I think that Comment::in_preview should do the same.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 2532776-13.patch | 786 bytes | abhisekmazumdar |
| #1 | 2532776-1.patch | 784 bytes | daffie |
Comments
Comment #1
daffie CreditAttribution: daffie commentedComment #2
andypostOverall +1 to issue
Would be great to check actual usage of the property and add beta evaluation
suppose
booldefault is
FALSEComment #3
daffie CreditAttribution: daffie commentedI copied the variable $in_preview from the Node entity. Berdir explained in #2525068: Document the class variable Node::in_preview as will stay public comment #3 why variable $in_preview is as it is.
Comment #8
andreyjan CreditAttribution: andreyjan at FFW commentedComment preview is currently failing with the follwoing error:
Drupal\Core\Entity\EntityMalformedException: The "comment" entity cannot have a URI as it does not have an ID in Drupal\Core\Entity\Entity->toUrl()
Applying #1 patch in combination with the patch from https://www.drupal.org/project/drupal/issues/2914233#comment-12289698 fixes the issue.
This can be also related to https://www.drupal.org/project/drupal/issues/2631246
Comment #13
abhisekmazumdarRe-rolled the #1 patch.
Comment #14
andypostComment #15
joachim CreditAttribution: joachim at Factorial GmbH commentedLGTM.
Comment #16
catch#2498919: Node::isPublished() and Node::getOwnerId() are expensive doesn't explain why there's a performance improvement compared to a protected property with a getter method, it just says that there is one. If we're going to commit this just for consistency, I think we should remove the performance claim here. The entity API in 2021 is not the same as the one in 2015 (and neither is PHP itself).
Additionally, the $in_preview property makes comment (and node rendering) tricky with caching.
The whole reason we have this is because of the way node and comment modules render links. We should refactor that code - it would probably need to respect isNew(), but setting a property on the entity is a bad way to pass render context around.
Comment #17
joachim CreditAttribution: joachim as a volunteer commented> doesn't explain why there's a performance improvement compared to a protected property with a getter method, it just says that there is one
I have to say, I was a bit confused by that too.
#2498919: Node::isPublished() and Node::getOwnerId() are expensive talks about accessing the property causing the flow to go into TypedData. But TypedData only gets invoked for a missing property via the magic __get(). If $node or $comment has the $in_preview property, then isn't that just a straight property access?
Comment #18
catch@alexpott pointed out that the magic __get() covers this too - in_preview ends up in a protected property that collects together any random non-field values. So declaring the property would mean that logic doesn't run, but still doesn't seem like much of a performance improvement since this logic is checked very rarely (only to build links).
This issue reminded me how much I'd like to get rid of the underlying logic in general... opened #3244848: Remove comment and node preview.