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
- Solve the WTF and improve discoverability by providing a
setDefaultRevision()
method onRevisionableInterface
. - Deprecate the setting functionality of
isDefaultRevision()
. - Update all existing code that calls
isDefaultRevision()
with the goal of setting the property to usesetDefaultRevision()
instead so this can serve as examples to developers. - Replace existing code that sets the
isDefaultRevision
property through the magic setter to usesetDefaultRevision()
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.
Comment | File | Size | Author |
---|---|---|---|
#3 | interdiff.txt | 1.94 KB | pfrenssen |
#3 | 2706337-3.patch | 9.9 KB | pfrenssen |
Issue fork drupal-2706337
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:
Comments
Comment #1
pfrenssenpfrenssen created an issue. See original summary.
Comment #2
pfrenssenComment #3
pfrenssenI added a dedicated test for completeness.
The old behaviour of passing the new state to
isDefaultRevision()
is still covered byContentEntityBaseUnitTest::testIsDefaultRevision()
.Also fixed a typo: wrote "property" instead of "argument" in the docblock.
Comment #5
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, that makes a lot more sense than setting with an is() method.
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #8
joachim CreditAttribution: joachim as a volunteer commentedIdeally, the $new_value parameter should be marked as deprecated, but our documentation standards don't suggest that parameters can have a @deprecated.
Comment #9
pfrenssenIndeed, 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.Comment #10
alexpottThis issue needs a change record and probably should file the Drupal 9 issue to fix isDefaultRevision().
Comment #11
pfrenssenRegarding 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 usingtrigger_error()
to throw aE_USER_DEPRECATED
warning. It would be nice to use this here too, that would make the deprecation more easily discoverable.Comment #13
cilefen CreditAttribution: cilefen commentedComment #22
alexpottI'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:
Comment #32
alexpott