Document the class variable Node::in_preview as will stay public

In case you think this is justifyable, this needs a CR.

I'm quite sure that for example @xjm would push that back.

This was done by design in #2498919: Node::isPublished() and Node::getOwnerId() are expensive to document an existing, used property. Defining it as a public property allows for a considerable performance improvement.

I really want to do that to the original property as well, it's better to have a documented public property than one that is not but is still used.

I'm OK with adding methods but I'm against making it protected, that's an API change and we're definitely past the time where we can make API changes just because it's cleaner.

1. +++ b/core/modules/node/src/Entity/Node.php
   @@ -77,6 +77,9 @@ class Node extends ContentEntityBase implements NodeInterface {
      /**
       * Whether the node is being previewed or not.
       *
   +   * The variable is set to public for it will result in a considerable
   +   * performance improvement. See https://www.drupal.org/node/2498919
   +   *
       * @var true|null
       */
      public $in_preview = NULL;
   

   Nice comments added here... But since it's public, this docblock should tell us what value corresponds to what state. Especially since it's not TRUE|FALSE but is instead TRUE|NULL.

   Also, the getter and setter never handle NULL, so is the @var annotation correct in the first place?

2. +++ b/core/modules/node/src/NodeInterface.php
   @@ -168,4 +168,22 @@ public function setRevisionAuthorId($uid);
   +   * Checks if the node is in preview mode.
   +   *
   +   * @return bool
   +   *   TRUE if the node is in preview mode.
   +   */
   +  public function isInPreview();
   +
   +  /**
   +   * Set if the node in to be viewed in preview mode.
   +   *
   +   * @param bool $in_preview
   +   *   Boolean value to indicate if the node in to be viewed in preview mode.
   +   *
   +   * @return $this
   +   */
   +  public function setInPreview($in_preview);
   

   Again, the interface docs don't match up with the property docs, making no mention of NULL.

   So, basically: Let's find out why that NULL is documented in there, and whether it's erroneous.

   I'm also inclined to say the property should be completely encapsulated as protected, but @berdir has spoken. :-)

RTBC +1 if green. Already discussed about the in_preview to be public in the issue.Do we need update at other place in Documenation ?

It is TRUE|NULL for BC.

There is code that's checking it with isset($node->in_preview). That is TRUE if the variable is FALSE.

@Mile23: Thanks for your review! Let me begin by stating that I fully agree with you. And I have no idea why they those for TRUE|NULL instead of TRUE|FALSE.

Yah, that's why we should look it up and find out and then know. :-)

It turns out there are a few places where we see things like if (empty($node->in_preview)), such as this from statistics.module:

  if (!$node->isNew() && $view_mode == 'full' && node_is_page($node) && empty($node->in_preview)) {

That's exactly the sort of unreadable junk we're trying to avoid by encapsulating in an object. But... There we are. :-)

Anyway.... That sort of code will work with either FALSE or NULL, since PHP allows that.

So:

Add to the property docblock that $is_preview should be TRUE if the node is being previewed, NULL otherwise.

I improved the english a bit in the comment. Otherwise looks good.

#10 + #11 Lets not scope creep this issue.

The issue is about missing documentation, not actually changing the actual usage of the in_preview variable. That should be handle in a separate issue.

It's not scope creep.

Since we're not doing for-reals encapsulation, we need to document the behavior so that it's clear.

#15 I see I misread your comment, my bad and I agree.

Anyways, we added the the description in the docblock, so looks all good now.

Cool, thanks. RTBC +1

Committed b843b4a and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.