Problem/Motivation

Way back in 2012 we added support for declaring revisions as the current revision: #218755: Support revisions in different states. This has been implemented with a getter and setter all rolled in the same method:

  /**
   * Checks if this entity is the default revision.
   *
   * @param bool $new_value
   *   (optional) A Boolean to (re)set the isDefaultRevision flag.
   *
   * @return bool
   *   TRUE if the entity is the default revision, FALSE otherwise. If
   *   $new_value was passed, the previous value is returned.
   */
  public function isDefaultRevision($new_value = NULL);

This is contrary to how we do things nowadays, the rest of core always uses dedicated getters and setters. This is confusing, and for people unfamiliar with the interface it is difficult to discover how a default revision can be set.

The confusion is made worse because in the example implementation in NodeRevisionsTest it appears that we are supposed to set the isDefaultRevision property directly, which is a WTF since this is a protected property. The below code only works because the write call that sets the isDefaultRevision property is intercepted by the magic __set() method in ContentEntityBase.

    // Make a new revision and set it to not be default.
    // This will create a new revision that is not "front facing".
    $new_node_revision = clone $node;
    $new_body = $this->randomMachineName();
    $new_node_revision->body->value = $new_body;
    // Save this as a non-default revision.
    $new_node_revision->setNewRevision();
    $new_node_revision->isDefaultRevision = FALSE; // Setting a protected property directly??!?
    $new_node_revision->save();

Proposed resolution

  1. Solve the WTF and improve discoverability by providing a setDefaultRevision() method on RevisionableInterface.
  2. Deprecate the setting functionality of isDefaultRevision().
  3. Update all existing code that calls isDefaultRevision() with the goal of setting the property to use setDefaultRevision() instead so this can serve as examples to developers.
  4. Replace existing code that sets the isDefaultRevision property through the magic setter to use setDefaultRevision() instead.

This probably doesn't require any new test coverage, this will be adequately tested by adapting the existing tests.

Remaining tasks

Implement.

User interface changes

None.

API changes

A new method setDefaultRevision() added to RevisionableInterface.

Data model changes

None.

Issue fork drupal-2706337

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

pfrenssen created an issue. See original summary.

pfrenssen’s picture

Status: Active » Needs review
FileSize
8.87 KB
pfrenssen’s picture

FileSize
9.9 KB
1.94 KB

I added a dedicated test for completeness.

The old behaviour of passing the new state to isDefaultRevision() is still covered by ContentEntityBaseUnitTest::testIsDefaultRevision().

Also fixed a typo: wrote "property" instead of "argument" in the docblock.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, that makes a lot more sense than setting with an is() method.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2706337-3.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community
joachim’s picture

   public function isDefaultRevision($new_value = NULL) {
     $return = $this->isDefaultRevision;
     if (isset($new_value)) {
-      $this->isDefaultRevision = (bool) $new_value;
+      $this->setDefaultRevision($new_value);

Ideally, the $new_value parameter should be marked as deprecated, but our documentation standards don't suggest that parameters can have a @deprecated.

pfrenssen’s picture

Indeed, we are using the tags from PHPDoc (formerly Doxygen) but the @deprecated can only be used to mark the entire method as deprecated. This is not the case here so we cannot use that tag. That's why I added the deprecation warning on the parameter documentation. I am using the same wording we typically use for deprecation warnings, hopefully this is clear for everyone.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This issue needs a change record and probably should file the Drupal 9 issue to fix isDefaultRevision().

pfrenssen’s picture

Regarding deprecating the parameter for isDefaultRevision(), this is something we also did recently in #2789315: Create EntityPublishedInterface and use for Node and Comment. In that issue this was solved very neatly by using trigger_error() to throw a E_USER_DEPRECATED warning. It would be nice to use this here too, that would make the deprecation more easily discoverable.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Issue summary: View changes

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

I've marked #2772155: The docs for ContentEntityBase::isDefaultRevision() do not adequately explain its purpose as a duplicate of this issue. I'll transfer issue credit.

Next steps here are:

  • We need to re-roll the patch here and update any other usages where we are using isDefaultRevision() as a setter.
  • We need to add a deprecation to isDefaultRevision() when it is called with an arguments.
  • We need to add scalar typehints and a return typehint to the new method.
  • We need to target 9.4.0 for the deprecation

alexpott credited Berdir.

alexpott credited jofitz.

alexpott credited tameeshb.

alexpott’s picture

Neslee Canil Pinto made their first commit to this issue’s fork.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.