Problem/Motivation

Currently we determine if a revision is the default by comparing the revision ID. For example CASE base.vid WHEN revision.vid THEN 1 ELSE 0 END AS isDefaultRevision compares the revision ID in the base table with the revision ID in the revision table.

A draft revision is considered to be a revision with a higher revision ID than the default.

This causes issues, especially with translations, where one translation in a revision may be a draft when others are not. It also prevents us from having draft revisions with a lower revision ID than the default.

It also prevents us from having an audit log of which revisions have been the default.

Proposed resolution

Store which revisions have been default via a revision_default revision metadata field and a method to return that value.

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Reviews

User interface changes

None

API changes

A new method RevisionLogInterface::wasDefaultRevision() has been added, returning the revision default status when it was stored.

Data model changes

A new revision_default field is added to all revisionable entity types.

CommentFileSizeAuthor
#93 entity-revision_default-2891215-93.patch23.34 KBeffulgentsia
#91 entity-revision_default-2891215-90.interdiff.txt4.86 KBplach
#91 entity-revision_default-2891215-90.patch23.58 KBplach
#85 entity-revision_default-2891215-84.interdiff.txt3.39 KBplach
#85 entity-revision_default-2891215-84.patch25.11 KBplach
#83 entity-revision_default-2891215-83.interdiff.txt886 bytesplach
#83 entity-revision_default-2891215-83.patch24.23 KBplach
#82 entity-revision_default-2891215-82.interdiff.txt3.26 KBplach
#82 entity-revision_default-2891215-82.patch24.18 KBplach
#74 entity-revision_default-2891215-74.interdiff.txt1.38 KBplach
#74 entity-revision_default-2891215-74.patch23.13 KBplach
#71 entity-revision_default-2891215-71.interdiff.txt6.52 KBplach
#71 entity-revision_default-2891215-71.patch23.12 KBplach
#67 entity-revision_default-2891215-67.interdiff.txt1.27 KBplach
#67 entity-revision_default-2891215-67.patch18.05 KBplach
#66 entity-revision_default-2891215-66.interdiff.txt12.45 KBplach
#66 entity-revision_default-2891215-66.patch18.03 KBplach
#64 entity-revision_default-2891215-64.interdiff.txt27.13 KBplach
#64 entity-revision_default-2891215-64.patch22.1 KBplach
#61 entity-revision_type-2891215-61.interdiff.txt648 bytesplach
#61 entity-revision_type-2891215-61.patch17.13 KBplach
#58 entity-revision_type-2891215-58.interdiff.txt2.94 KBplach
#58 entity-revision_type-2891215-58.patch17.04 KBplach
#53 entity-revision_type-2891215-53.interdiff.txt737 bytesplach
#53 entity-revision_type-2891215-53.patch16.01 KBplach
#50 entity-revision_type-2891215-50.patch15.98 KBplach
#50 entity-revision_type-2891215-50.interdiff.txt7.88 KBplach
#49 entity-revision_type-2891215-49.interdiff.txt6.25 KBplach
#49 entity-revision_type-2891215-49.patch14.1 KBplach
#48 entity-revision_type-2891215-48.interdiff.txt2.66 KBplach
#48 entity-revision_type-2891215-48.patch14.66 KBplach
#47 entity-revision_type-2891215-47.patch16.71 KBplach
#47 entity-revision_type-2891215-47.interdiff.txt17.46 KBplach
#46 entity-revision_type-2891215-46.patch12.61 KBplach
#46 entity-revision_type-2891215-46.interdiff.txt7.98 KBplach
#45 entity-revision_type-2891215-45.interdiff.txt7.63 KBplach
#45 entity-revision_type-2891215-45.patch11.58 KBplach
#34 entity-revision_type-2891215-33.interdiff.txt10.72 KBplach
#33 entity-revision_translation_api-2929496-13.interdiff.txt1.79 KBplach
#33 entity-revision_type-2891215-33.patch11.8 KBplach
#27 entity-revision_type-2891215-26.interdiff.txt1.42 KBplach
#27 entity-revision_type-2891215-26.patch11.66 KBplach
#25 entity-revision_type-2891215-25.patch10.6 KBplach
#11 2891215-11.patch5.54 KBtimmillwood
#11 interdiff-2891215-11.txt7.5 KBtimmillwood
#3 2891215-2.patch5.22 KBtimmillwood
#2 2890364-6.patch24.01 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Assigned: timmillwood » Unassigned
Status: Active » Needs review
FileSize
24.01 KB

Very early patch.

timmillwood’s picture

Helps if I upload the patch for the correct issue!

timmillwood’s picture

Some initial questions:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -375,6 +382,10 @@ public function preSave(EntityStorageInterface $storage) {
    +      $this->set('revision_type', $this->isDefaultRevision() ? 'default' : 'draft');
    

    Not sure presave is the best place for this. Not sure if we want to keep up dating the field value through the changes in the entity, or just set it before saving.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1234,6 +1245,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      $fields['revision_type'] = BaseFieldDefinition::create('string')
    

    Should this be an entity_key? Should it be not null? Would a boolean field be better?

  3. +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
    @@ -40,6 +40,14 @@ public function setNewRevision($value = TRUE);
    +  public function getRevisionType();
    

    Should this be in it's own interface?

  4. +++ b/core/modules/system/system.install
    @@ -1984,3 +1984,19 @@ function system_update_8401() {
    +function system_update_8402() {
    

    This is in system.install for now so it will update contrib. If we switch to using an entity_key we can just add the update hook to the modules which implement the key.

  5. +++ b/core/modules/system/system.install
    @@ -1984,3 +1984,19 @@ function system_update_8401() {
    \ No newline at end of file
    

    oops!

Status: Needs review » Needs work

The last submitted patch, 3: 2891215-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

plach’s picture

1: Are you concerned that this might affect affected translations and in the future dirty field detection?
2: Yep, I think this should be an entity key and the field name we come up with should only be the entity key's default value. This is not going to optional IMHO. If we make it an entity key it should be automatically flagged as not null.
3: I think it's fine to leave it there, especially if we switch to a boolean. In this case RevisionableInterface::isDefaultRevision() should actually return the field value, unless overwritten. By the way, it would be nice to deprecate the update behavior and use a regular setter for that.
4: As I was saying above I'm not sure it's worth to make this optional. We're going to need to support both cases if it is and it's not going to be fun :)

timmillwood’s picture

Assigned: Unassigned » timmillwood

@plach

  1. I hadn't thought about affected translations or dirty fields. If a revision is type X, it stays that type until isDefaultRevision($value) is called to change that. Therefore when an entity object is created, should the field return default, until changed in isDefaultRevision()? and if editing a revision should the field stay as default/draft until changed? or do we just update it in preSave based on the isDefaultRevision property?
  2. Ok, makes sense.
  3. Yes, I think this would be a good time to rejuvinate the isDefaultRevision() method.
  4. ok, so this should stay in system.install and be applied to all revisionable entity types (core and contrib).
hchonov’s picture

1. I don't think we should do anything in the presave. CEB::isDefaultRevision(TRUE) should be setting the flag to "default" and content moderation should be setting it to "draft".
2. Additionally this is a revision metadata field and should be added accordingly to the annotation in order to make the field available in CEB::getFieldsToSkipFromTranslationChangesCheck().
3. The new method should not be placed in RevisionableInterface, but in ContentEntityInterface, because we have a policy that a new method is API break and new methods are allowed only in interfaces where a 1:1 relationship between an interface and a class is provided, in which case developers should be extending from the base class instead implementing the interface.
4. We are going to need a default in ContentEntityType::getKeys, right? Because the field will be installed in custom and contrib, but not available. We've already done something similar with the revision metadata keys.

timmillwood’s picture

@hchonov

  1. I'm not sure I agree Content Moderation should be setting it to draft, because other modules can also create draft revisions by calling isDefaultRevision(False).
  2. Revision metadata fields are not translatable. So not sure we can do that, unless we rework them.
  3. Ok, noted!
plach’s picture

To address 1) and 3) why don't we actually switch to a boolean field and call it default_revision? That way we can legitimately use ::isDefaultRevision() as the accessor method and set/store the field value using the regular entity save workflow. This would also match the default_langcode boolean field, so it would be one (small) step forward in trying to bring some consistency between the translation and the revision subsystems.

@hchonov:

I was speaking about this with @catch a few days ago and he suggested that contrib can add more states via a new field if needed, but that in core there is no use case for more than a boolean, at least we could not come up with one. What use cases do you have in mind that require a multistate field?

timmillwood’s picture

FileSize
7.5 KB
5.54 KB

Updated the patch to be boolean, and also deprecating the param in isDefaultRevision(), then adding setDefaultRevision().

\Drupal\Tests\system\Functional\Entity\EntityRevisionsTest::testEntityRevisionParamConverter has uncovered an interesting question. English entity, add German, so now revision 2 is the default and has english and german both default. Add a forward revision of english, but german is still denoted as default. I guess this is right?

hchonov’s picture

Re #9:

I'm not sure I agree Content Moderation should be setting it to draft, because other modules can also create draft revisions by calling isDefaultRevision(False).

Everyone who is creating a non-default draft revision should be setting the revision type explicitly to draft, not only content moderation :).

Revision metadata fields are not translatable. So not sure we can do that, unless we rework them.

Why should the revision_type field be translatable? It should not be, therefore it should be ok to be a revision metadata key.

Re #10:

@hchonov:

I was speaking about this with @catch a few days ago and he suggested that contrib can add more states via a new field if needed, but that in core there is no use case for more than a boolean, at least we could not come up with one. What use cases do you have in mind that require a multistate field?

Yes, I actually have also other use case where I really wanted to have such a feature - one example is the autosave_form module, where I was considering to save autosave states as revisions of the revision_type "autosave". Another example was that on entity inline forms I create non-default revisions while the user is working on the page in order to accelerate the saving process later if having a big nested entity structure where at form save 100 entities with 10 translations will be saved. I see a lot of possible use cases for having a string instead of boolean revision_type field.

catch’s picture

That way we can legitimately use ::isDefaultRevision() as the accessor method

I'm not sure we can do that. If you load an old revision that used to be the default, then the value of this field should indicate that it was, but it won't currently be the default revision. A wasDefaultRevision() (not suggesting that name just can't think of anything else) would always be accurate though.

Yes, I actually have also other use case where I really wanted to have such a feature - one example is the autosave_form module, where I was considering to save autosave states as revisions of the revision_type "autosave".

If something's saved with a value of 'autosave' instead of the boolean, how would we know whether it was the default revision at the time it was saved or not?

timmillwood’s picture

@plach @hchonov and I just talked at length about this issue.

We discussed if the if the field we add should be boolean or string. @hchonov had some good ideas why it should be a string, similar to as he stated in #12.

Then we talked about if the field should be revision metadata. I argued that if it was metadata then all translations in the revision must be of the same type. @hchonov and @plach convinced me that this is correct, because the revision as a whole, is either default or not. The issue there though is for #2766957: Forward revisions + translation UI can result in forked draft revisions it doesn't denote which of the translations in the revision is the draft.

We went on to discuss that the revision_translation_affected (RTA) field is there to denote which translation is changed, and thus which translation the revision is for. I stated that I've been able to replicate a case where all translations are marked as RTA NULL. This is possible by not making any changes to an entity when saving, and because the moderation state added by Content Moderation is a computed field, this doesn't qualify as a change.

From here we talked about various options such as changing the RTA field to denote which translation the new revision was for, even if there were no changed. Although this could be a backwards compatibility break. We looked at if we could store a further revision metadata field to store the main language of a revision. However in most use cases this giving the same functionality as the RTA field, and how do we handle cases where multiple translations change in a revision.

The closing thought was about if a revision is RTA NULL for all translations we go back through the revisions until we find one RTA 1, then use this. This should be used in Content Moderation for the entity form and also the latest revision tab. It would also solve the most recent issues seen in #2766957: Forward revisions + translation UI can result in forked draft revisions.

@plach and I briefly talked about draft published revisions. This is not possible with Content Moderation out of the box, but it is possible to create a new workflow state that is published, but doesn't create a default revision, and therefore can create draft published revisions. This would only be possible from revision 2 onwards, because revision 1 always has to be set as default. It is not possible to have an entity with no default revision.

@plach and I then talked on IRC after the call about Content Moderation updating the RTA value. I suggested that in \Drupal\content_moderation\EntityOperations::entityPresave where we update the default revision, we could also update the RTA flag if the moderation state has changed.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1234,6 +1248,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+        ->setInitialValue(TRUE)

I'm not sure this is correct, it will mark every revision as default when we install the new base field..

hchonov’s picture

Re #13:

If something's saved with a value of 'autosave' instead of the boolean, how would we know whether it was the default revision at the time it was saved or not?

The main idea of autosave is that it is only provided for the entity forms working with the default revision. We don't need to know if it was the default revision or not, but even if we for some reason need to then that could be achieved through the parent_revision field we've mentioned we'll soon probably need. If and it will happen that a new default revision and probably by some other user is saved after the autosave revisions have been created and in this case we'll need the conflict management solution in order to merge two entity revisions and make an autosaved one a default revision. Here is where the conflict module should be used.

Re #14:

@plach and I then talked on IRC after the call about Content Moderation updating the RTA value. I suggested that in \Drupal\content_moderation\EntityOperations::entityPresave where we update the default revision, we could also update the RTA flag if the moderation state has changed.

I would like to mention it again that I am not really convinced by setting that flag manually to TRUE, because the idea of this flag was that it is computed based if there are real editorial changes on the content fields of a translation. Setting it manually in order to spare us some work on introducing a revision_type feels not really right. I am pretty sure that a revision_type field might and will be really useful. The idea is that even if you solve it through setting the field manually how could one say if a previous revision was a default or a draft one? Out there are systems where the editorial history is important.

plach’s picture

@hchonov:

Setting it manually in order to spare us some work on introducing a revision_type feels not really right.

Well, in my mind manually setting the RTA flag is not about not doing the revision_type change, it is about not doing the revision_langcode one. As I mentioned in the call, we already have all sorts of language fields and methods scattered through out the Entity Field API. Introducing a "revision language" concept when we support multiple languages per revision would further increase the confusion.

In IRC @timmillwood clarified that the moderation state is actual entity metadata, it's only implemented as a computed field to overcome some technical difficulties. So when you change the moderation state conceptually you are actually changing the entity and we agreed that it would be good if CM integrated with Diff to make this visible in diffs.

IMHO the revision type information is not actually useful to address #2766957: Forward revisions + translation UI can result in forked draft revisions, and we should keep that part of the problem out of the discussion we are having in this issue, which should probably focus mainly around the string vs boolean aspect :)

plach’s picture

@amateescu:

I'm not sure this is correct, it will mark every revision as default when we install the new base field..

Didn't we support only the "default" revision type in core so far? The current default revision is the one referenced in the entity base table.

timmillwood’s picture

Didn't we support only the "default" revision type in core so far? The current default revision is the one referenced in the entity base table.

@amateescu is correct. I put that in the code with the idea that this field will be "NOT NULL" and we'd need to initially set all as default, then switch the draft ones (revision ID greater than the current default revision) to draft during the update hook. Although none of that has been implemented yet.

plach’s picture

Ah, I had no idea we already supported creating drafts :)

hchonov’s picture

Re #17:

we should keep that part of the problem out of the discussion we are having in this issue, which should probably focus mainly around the string vs boolean aspect :)

I fully agree with you, somehow I was thinking that this feature will be refused because the other issue has another way of solving the initial problem now. But actually we need that feature and yes, we should focus only on it.

I guess we should wait and see what @catch thinks about my answer of his question in #16.

Should we summarise pros and cons for string vs boolean?

plach’s picture

Should we summarise pros and cons for string vs boolean?

Yep, sounds like a good idea :)

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.

plach’s picture

Assigned: timmillwood » plach

I'm doing some work on this subject as part of #2860097: Ensure that content translations can be moderated independently. I will post an updated patch for this ASAP.

plach’s picture

FileSize
10.6 KB

Here is an updated patch for this. Unfortunately I forgot that @timmillwood already provided one in #3, so there are a few differences. However I'm happy to incorporate any relevant change once we decide on a way forward. For now I just implemented a boolean flag, but switching to a string would be trivial. Also the naming is still up for discussion. If we go with a boolean @catch and I were leaning towards something like ::wasDefaultRevision() or ::hasBeenDefaultRevision(), but happy to discuss alternatives.

timmillwood’s picture

I think the field should be called default_revision if we're having a boolean, because revision_type: TRUE doesn't really mean much.

From earlier discussions we looked at adding this field so we know which are pending revisions and which are or have been the default, rather than depending on sequential revision IDs. The issue here is if we have revision:1 which is default, then revision:2 which is pending, once we "publish" revision:2 we get revision:3 which is default, but revision:2 is still in the database as revision_type: FALSE, so will this be seen as a "pending revision"?

plach’s picture

plach’s picture

Oops, missed #26:

I think the field should be called default_revision if we're having a boolean, because revision_type: TRUE doesn't really mean much.

Yeah, I spent no time on the naming, since I knew we would be discussing that in detail here, along with the actual type. The only problem with default_revision is that it would have a slightly different meaning than ::isDefaultRevision(), which could be highly confusing.

The issue here is if we have revision:1 which is default, then revision:2 which is pending, once we "publish" revision:2 we get revision:3 which is default, but revision:2 is still in the database as revision_type: FALSE, so will this be seen as a "pending revision"?

In #2860097: Ensure that content translations can be moderated independently it's only seen as a revision not having been default. Over there a pending revision is the latest (affected) revision when it's not of default type.

plach’s picture

Assigned: plach » Unassigned
phenaproxima’s picture

How about naming it pending_revision, with the corresponding method being isPendingRevision()?

plach’s picture

Assigned: Unassigned » plach
Issue tags: +API addition

This morning there was an hangout call with @amateescu, @catch, @hchonov, @timmillwood and me. We discussed a few scenarios where a revision type would be useful. We all agreed there can be legitimate use cases for it, possibly even in core, however we could not conclusively come up with an example that would require such field in the short term. For this reason we decided to proceed with a boolean flag (default_revision: 0/1) and evaluate the need for an additional revision type field in a later stage, when we either require it in core, or there is a strong use case in contrib. This new field would typically be used to describe the "owner" module, but it actually would be a machine name, so more complex use cases could be addressed.

I'm going to update the latest patch ASAP.

plach’s picture

@phenaproxima:

We did not settle on a definitive method name yet, more discussion is welcome, however we are currently going with ::wasDefaultRevision().

plach’s picture

Here is an updated version. We still need some test coverage, as well as introducing a default_revision entity key and maybe tweaking the table mapping so that the field lives in the revision table.

plach’s picture

And now the correct interdiff...

plach’s picture

Assigned: plach » Unassigned
Issue tags: +Needs tests
plach’s picture

hchonov’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -331,6 +331,17 @@ public function isDefaultRevision($new_value = NULL) {
+    throw new \LogicException('The "default_revision" field data is missing for entity ' . (int) $this->id() . '.');

I hope this is only there for debugging purposes?

plach’s picture

I hope this is only there for debugging purposes?

The update is filling that field for all revisionable entity types, so I think it's correct to throw an exception if the field is not populated.

hchonov’s picture

The update is filling that field for all revisionable entity types, so I think it's correct to throw an exception if the field is not populated.

In which case could this happen?

plach’s picture

Well, in theory it should never happen, hence the exception :)

Unless maybe the method is invoked on a non-revisionable entity type, but probably a different exception would be thrown at that point, because the field would be missing.

timmillwood’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -331,6 +331,17 @@ public function isDefaultRevision($new_value = NULL) {
    +    throw new \LogicException('The "default_revision" field data is missing for entity ' . (int) $this->id() . '.');
    

    Why (int)? What if it's a string ID? Should we also provide the entity type ID in the exception?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -106,4 +106,12 @@ public function getLoadedRevisionId();
    +  public function wasDefaultRevision();
    

    Should we document that it throws a LogicException?

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    --- a/core/lib/Drupal/Core/Entity/EntityFieldManager.php
    +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
    
    +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
    @@ -221,6 +221,17 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +      $base_field_definitions['default_revision'] = BaseFieldDefinition::create('boolean')
    

    Why did you decide this should be a field added in EntityFieldManager rather than in ContentEntityBase?

  4. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -374,7 +374,6 @@ protected function runUpdates() {
    -      $this->assertFalse($needs_updates, 'After all updates ran, entity schema is up to date.');
    
    @@ -383,6 +382,7 @@ protected function runUpdates() {
    +      $this->assertFalse($needs_updates, 'After all updates ran, entity schema is up to date.');
    

    Although this looks to be a good change to UpdatePathTestBase, it's unrelated, so maybe should go in a separate issue?

hchonov’s picture

Well, in theory it should never happen, hence the exception :)

Unless maybe the method is invoked on a non-revisionable entity type, but probably a different exception would be thrown at that point, because the field would be missing.

I would say that we have to set a constraint on the field that it is required instead of throwing a logic exception if for any reason somehow it is not set when retrieving it.

Also as we are going to always set the field in the storage why should it have a default value?

timmillwood’s picture

I guess it would only be not set if isDefaultRevision() returns NULL, is that possible?

hchonov’s picture

isDefaultRevision() always returns a boolean, so this could not happen.

plach’s picture

Quickly addressed the reviews above, everything else is still @todo.

plach’s picture

This should fix the test failures. I had to remove the required field constraint as it was breaking a lot of validation tests, which would mean lots of BC breaks. I restored the exception and I'm fine with being there.

plach’s picture

This adds the "revision_default" revision metadata flag.

plach’s picture

Cleaned-up obsolete cruft

plach’s picture

This should have less failures.

plach’s picture

Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
7.88 KB
15.98 KB

This should be ready for review.

plach’s picture

Issue summary: View changes

Updated the IS

Status: Needs review » Needs work

The last submitted patch, 50: entity-revision_type-2891215-50.patch, failed testing. View results

plach’s picture

This should fix the last failure, everything else is caused by testbots not cooperating.

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 53: entity-revision_type-2891215-53.patch, failed testing. View results

plach’s picture

Status: Needs work » Needs review
gabesullice’s picture

Status: Needs review » Needs work

Overall, this looks really good!

One last, minor thing...

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -54,7 +54,7 @@ protected function checkStorageClass($class) {
+    if ($include_backwards_compatibility_field_names && count($this->revision_metadata_keys) < 4) {

Checking for a count < 4 seems arbitrary and is a subtle thing that will need to be maintained. Is there a way to write this in a way that it explains itself?

$required_keys = ['my', 'four', 'expected', 'keys'];
if (empty(array_diff($this->revision_metadata_keys, $required_keys))) { // ...
plach’s picture

Status: Needs work » Needs review
FileSize
17.04 KB
2.94 KB

Thanks, this should be better.

gabesullice’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the fix :)

amateescu’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
    @@ -54,19 +54,22 @@ protected function checkStorageClass($class) {
    -    if (!$this->revision_metadata_keys && $include_backwards_compatibility_field_names) {
    ...
    +    if ($include_backwards_compatibility_field_names) {
    

    We're losing the small optimization of checking the statically cached revision metadata keys here, is there any reason for that?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
    @@ -54,19 +54,22 @@ protected function checkStorageClass($class) {
    +        'revision_default' => 'revision_default',
    

    All the other revision metadata keys are handled in the "include backwards compatibility field names" branch because there were existing fields for some entity types, but I don't think that's the case for the new 'revision_default' field.

    I think we need to move it out of the BC if branch and have individual update functions that add the new entity revision metadata key to each revisionable entity type.

plach’s picture

Status: Needs work » Needs review
FileSize
17.13 KB
648 bytes
  1. We are not losing it, it's just moved a few lines below: all the "heavy" stuff is still done just once. I added an else branch to make sure this is the case also when the field is not defined.
  2. I don't understand the following suggestion:

    I think we need to move it out of the BC if branch and have individual update functions that add the new entity revision metadata key to each revisionable entity type.

    How can we add annotation keys via an update function? Also, I'm not sure I understand what's wrong with the current update function. It's true it's unlikely we have existing fields, but we are automatically providing them for any class using RevisionLogEntityTrait, so we need an update function to deal with any added field in a generic fashion.

Status: Needs review » Needs work

The last submitted patch, 61: entity-revision_type-2891215-61.patch, failed testing. View results

amateescu’s picture

How can we add annotation keys via an update function? Also, I'm not sure I understand what's wrong with the current update function. It's true it's unlikely we have existing fields, but we are automatically providing them for any class using RevisionLogEntityTrait, so we need an update function to deal with any added field in a generic fashion.

Ok, let me clarify my concern a bit :) What I wanted to say is that we shouldn't add the new field in the "provide backwards compatible revision metadata field names" branch of the ContentEntityType::getRevisionMetadataKeys(), but add the new revision metadata key through the entity definition update manager:

  $revision_metadata_keys = $entity_type->get('revision_metadata_keys');
  $revision_metadata_keys['revision_default'] = 'revision_default';
  $entity_type->set('revision_metadata_keys', $revision_metadata_keys);
  $definition_update_manager->updateEntityType($entity_type);

Also, are we sure that this field is needed only if the entity type implements RevisionLogInterface? Isn't the concept of "was default revision" something needed by all revisionable entity types, like 'revision_translation_affected' for example?

plach’s picture

Discussed this with @amateescu in Slack: we agreed to go back to the initial approach where we defined the revision_default flag in the entity field manager, since it's going to be a required bit of the core revision logic, pretty much as the revision_translation_affected flag. The main difference with the previous patches is that now the field lives in the revision table.

(Good luck reviewing the interdiff ;)

amateescu’s picture

This looks much better! Thanks to the review-bait above, I did not even open the interdiff :P

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -331,6 +331,27 @@ public function isDefaultRevision($new_value = NULL) {
    +    if (!$entity_type->isRevisionable()) {
    +      throw new \LogicException("Entity type {$this->getEntityTypeId()} does not support revisions.");
    +    }
    

    Other revision-related methods do not throw exceptions for non-revisionable entity types, they just return a value that makes sense in each context.

    In this case, I think the correct return value is TRUE for non-revisionable entity types.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -331,6 +331,27 @@ public function isDefaultRevision($new_value = NULL) {
    +    $revision_default_key = $entity_type->getKey('revision_default');
    

    I assume the field name revision_default was chosen when this was a revision metadata key to match the rest of the field names, but now that it's an entity key, how about renaming it to default_revision to match default_langcode?

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -331,6 +331,27 @@ public function isDefaultRevision($new_value = NULL) {
    +    $value = $this->isNew() ? $this->isDefaultRevision() : $this->get($revision_default_key)->value;
    

    Can't we just return TRUE when the entity is new?

  4. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
    @@ -220,6 +221,15 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +    // Make sure revisionable entity types are correctly defined.
    ...
         // Make sure that revisionable entity types are correctly defined.
    

    These two comments are kind of the same right now, can we change the one below to "Make sure that revisionable and translatable entity types are correctly defined"?

    And the first comment is missing "that" :)

  5. +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
    @@ -51,6 +51,18 @@ public function getRevisionId();
    +   * @throws \LogicException
    +   *   If the entity type is not revisionable or the default revision data is
    +   *   missing.
    

    If you agree with the point above, this exception will be thrown only when the default revision data is missing.

    However, this makes me wonder: how will it be possible for this data to be missing?

  6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -330,7 +330,14 @@ public function getTableMapping(array $storage_definitions = NULL) {
    -      $revision_metadata_fields = $revisionable ? array_values($this->entityType->getRevisionMetadataKeys()) : [];
    +      $revision_metadata_fields = [];
    +      if ($revisionable) {
    +        $revision_metadata_fields = array_values($this->entityType->getRevisionMetadataKeys());
    +        $revision_default_key = $this->entityType->getKey('revision_default');
    +        if ($revision_default_key) {
    +          array_unshift($revision_metadata_fields, $revision_default_key);
    +        }
    +      }
    

    This is a bit worrying. I was hoping that by introducing the revision metadata keys concept we wouldn't need to hardcode any other field name in here.

    Does this mean that the new field should actually be a revision metadata key?

  7. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
    @@ -245,6 +245,7 @@ protected function copyData(array &$sandbox) {
    +    $revision_default_affected_key = $temporary_entity_type->getKey('revision_default');
    

    $revision_default_affected_key => $revision_default_key, but it should actually be $default_revision_key based on the second review point.

plach’s picture

Addressed #65.

I kept revision_default as the field name, since default_revision may be confusing, given we have also ::isDefaultRevision(). Additionally this is consistent with the other revision metadata keys.

plach’s picture

amateescu’s picture

  1. +++ b/core/modules/system/system.install
    @@ -2056,3 +2056,47 @@ function system_update_8403() {
    +  $definitions = array_filter(\Drupal::entityTypeManager()->getDefinitions(), function (EntityTypeInterface $entity_type) use ($definition_update_manager) {
    ...
    +      $definition_update_manager->updateEntityType($entity_type);
    

    Shouldn't we update the entity type definition with a $entity_type->set('revision_metadata_keys') before updating it in the definition update manager?

    As written now, this code will update the "last installed entity type definition" with everything that exists in the "live" definition, which might include other changes that are unrelated to this new revision metadata key.

  2. +++ b/core/modules/system/system.install
    @@ -2056,3 +2056,47 @@ function system_update_8403() {
    +    // Install the 'revision_translation_affected' field if needed.
    

    Stale comment, should mentioned 'revision_default' now :)

The last submitted patch, 66: entity-revision_default-2891215-66.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 67: entity-revision_default-2891215-67.patch, failed testing. View results

plach’s picture

This should fix the last failures, address #68, and add explicit test coverage for the update.

Status: Needs review » Needs work

The last submitted patch, 71: entity-revision_default-2891215-71.patch, failed testing. View results

amateescu’s picture

  1. +++ b/core/modules/system/system.install
    @@ -2079,10 +2079,14 @@ function system_update_8501() {
    +      $installed_entity_type->set('revision_metadata_keys', $revision_metadata_keys);
           $definition_update_manager->updateEntityType($entity_type);
    

    You forgot to use $installed_entity_type here :)

  2. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateAddRevisionDefaultTest.php
    @@ -0,0 +1,90 @@
    +class EntityUpdateAddRevisionDefault extends UpdatePathTestBase {
    

    This class needs a "Test" suffix.

plach’s picture

Geez... I need some sleep :D

(Btw, I blame PHP Storm for the wrong test class name: it's able to run a test even without autoloading working properly ;)

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great now!

hchonov’s picture

I see that @amateescu asked as well in #65.5 how could it happen that the exception is thrown i.e. how could it happen that the default revision data is missing? I think if we are throwing an exception we should document in which cases it will be thrown and we still don't know when this could happen.

plach’s picture

AFAICT that exception can be thrown only if the update goes wrong or someone manually fiddles with the database. Not sure whether adding this would improve the documentation significantly.

plach’s picture

Or maybe a faulty migration...

hchonov’s picture

This could happen everywhere, but we are not throwing an exception in each getter method. If so then we should provide a general method "ensureValueExists" and run each each retrieved value through it, at least the ones of required fields, but I think this is unnecessary complex.

plach’s picture

I didn't examine existing cases, but here we want to avoid interpreting a missing value as FALSE, in this case I'd rather be inconsistent than sorry, especially given that we are talking about an uncommon scenario.

effulgentsia’s picture

I agree with #79 that we shouldn't have individual one-off code for throwing exceptions when required fields are NULL. I think it's the storage's responsibility to enforce required. For SQL storage, I think we already do that via NOT NULL SQL constraints. For contrib/custom no-SQL back-ends that can't enforce it at the storage engine level, the PHP class may need to throw exceptions on either writing or reading, and should do so for all required fields.

However:

+++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
@@ -221,6 +223,17 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
+    if ($entity_type->isRevisionable()) {
+      $field_name = $entity_type->getRevisionMetadataKey('revision_default');
+      $base_field_definitions[$field_name] = BaseFieldDefinition::create('boolean')
+        ->setLabel($this->t('Default revision'))
+        ->setDescription($this->t('A flag indicating whether this was a default revision when it was saved.'))
+        ->setTranslatable(FALSE)
+        ->setRevisionable(TRUE);
+    }

Looks to me like we're missing a setRequired()?

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
24.18 KB
3.26 KB

Setting the field as required does not work, because it makes entity validation fail.

This patch triggers a PHP error instead of an exception, when the value is missing, and returns a (kind of) sensible default, which is the next best thing to do IMO to avoid failing silently. I hope this is good now because I'm out of ideas otherwise and I don't think it's worth going down a rabbit hole for a tiny detail, given the amount of work this is blocking.

plach’s picture

effulgentsia’s picture

Setting the field as required does not work, because it makes entity validation fail.

Hm, we ought to have a way of dealing with this. Not sure if it means setting the value during validation, or somehow opting out a required field from having the validation constraint (on the grounds that it will get set during saving). Seems kind of similar to a computed field, but maybe not exactly. I guess we can figure it out in a follow-up though rather than holding up this issue on it.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -375,6 +375,13 @@ protected function doSave($id, EntityInterface $entity) {
+      $entity->set($revision_default_key, $entity->isDefaultRevision());

Does this mean that if a revision is saved as a default, then later resaved when not a default, then this will set it to FALSE and wasDefaultRevision() will return FALSE? Is that what's desired on the basis that if an old revision is resaved we want to treat its new state as never having been the default revision? Or should this line only be run when revision_default is not already TRUE on the basis that the revision ID was at one time the default regardless of later saves?

plach’s picture

Hm, we ought to have a way of dealing with this. [...]

I agree, this is similar to the concept of "internal" field @Wim Leers was mentioning at #2933518: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this.

Does this mean that if a revision is saved as a default, then later resaved when not a default [...]

Good catch! We don't have anything enforcing this logic currently, however you cannot make the default revision not default, unless you save a new revision, because otherwise you'd have no default revision. That said, better offering actual protection against this case, the latest patch should achieve that by populating the flag only when a new revision is saved.

catch’s picture

This is looking good to me now, and glad we went for the minimal API here with the option to expand it later.

plach’s picture

Assigned: Unassigned » effulgentsia
Status: Needs review » Reviewed & tested by the community

Cool, patch green. Setting back to RTBC as #84 is addressed and @catch is ok with the fix per #86.

Back to Alex for a final confirmation.

hchonov’s picture

Setting the field as required does not work, because it makes entity validation fail.

Will the entity validation fail even if a default value is set?

plach’s picture

Probably it wouldn't, but I feel that "required" is the wrong concept here: that means that the field has always to be populated, even before the entity is being stored, which is not the case. I think "internal" is what we want here, which may very well imply a NOT NULL clause as well, possibly by being marked required. However in this case it would be clear that the user is not supposed to provide its value and validation could be skipped for internal fields.

amateescu’s picture

@plach, we have "storage required" for exactly that use case: when we want the field to not be required at validation time, but it has to have a value when it is stored.

plach’s picture

Brilliant, this is exactly what we needed!

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Glad I could help :) #91 is RTBC for me if the testbot agrees.

plach’s picture

Added change record at https://www.drupal.org/node/2936349.

plach’s picture

Title: There is no way of knowing if a revision is or was a draft / default revision » Add a way to track whether a revision was default when originally created
Issue summary: View changes

Updated IS

plach’s picture

Issue summary: View changes
effulgentsia’s picture

Removing self credit (#93 was just a straight reroll) and ticking credit boxes for reviewers.

effulgentsia’s picture

Seems unchecking my own credit checkbox isn't working at the moment. I'll remove it from the git commit, and if d.o. is fixed at some point to allow unchecking it in the future, I'll try to remember to do so (or someone else with permissions: feel free to).

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.6.x and cherry picked to 8.5.x.

plach’s picture

Awesome, thank you!

Berdir’s picture

This broke the revision metadata fields BC layer by initializing revision_metadata_keys in the constructor which breaks the condition in \Drupal\Core\Entity\ContentEntityType::getRevisionMetadataKeys().

I assume it will be a easy as moving that default into the getter method, will open a follow-up.

Berdir’s picture

Follow-up: #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key

That was the third and hopefully last problem that I found while testing 8.5.x today :)

jonathan1055’s picture

@effulgentsia Regarding the comment in #99 can you give a link to the commit(s) please? I think this change is the cause of Autocomplete test failures in Rules module. Thanks.

timmillwood’s picture

Assigned: effulgentsia » Unassigned
effulgentsia’s picture

@jonathan1055: Sorry if this broke something in Rules module. Please comment here with what you learn about that. I wonder if it's the same thing as #102 or something different. Alpha will be tagged this week, so I'd like to either revert this commit or fix critical regressions caused by it prior to that.

plach’s picture

@jonathan1055:

Please test the latest patch at #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key and see if that fixes your issue, it should be RTBC very soon.

Wim Leers’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContent/BlockContentResourceTestBase.php
@@ -122,6 +122,11 @@ protected function getExpectedNormalizedEntity() {
+      'revision_default' => [
+        [
+          'value' => TRUE,
+        ],
+      ],

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
@@ -212,6 +212,11 @@ protected function getExpectedNormalizedEntity() {
+      'revision_default' => [
+        [
+          'value' => TRUE,
+        ],
+      ],

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
@@ -152,6 +152,11 @@ protected function getExpectedNormalizedEntity() {
+      'revision_default' => [
+        [
+          'value' => TRUE,
+        ],
+      ],

+++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
@@ -150,6 +150,9 @@ public function testNormalize() {
+      'revision_default' => [
+        ['value' => TRUE],
+      ],

@@ -225,6 +228,7 @@ public function testSerialize() {
+      'revision_default' => '<revision_default><value>1</value></revision_default>',

These are HTTP API additions we have to support forever if we're not careful!


#84:

Hm, we ought to have a way of dealing with this. Not sure if it means setting the value during validation, or somehow opting out a required field from having the validation constraint (on the grounds that it will get set during saving). Seems kind of similar to a computed field, but maybe not exactly. I guess we can figure it out in a follow-up though rather than holding up this issue on it.

#85:

Hm, we ought to have a way of dealing with this. [...]

I agree, this is similar to the concept of "internal" field @Wim Leers was mentioning at #2933518: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this.

… yes indeed, and I'm unpleasantly surprised that this patch was able to land, change API responses, and I was never even notified.

I guess we can figure it out in a follow-up though rather than holding up this issue on it.

No! Things that affect the HTTP API (REST/JSON API/GraphQL) cannot be addressed at some point in the future. They need to be addressed before commit. And if not before commit, then at least before the minor ships in which this data would be exposed. Because once exposed, we may not ever be able to remove it.

Can we open a critical follow-up to mark this new base field as setInternal() by default? Only for a critical, it is guaranteed to be addressed before the minor is tagged AFAIK.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
@@ -380,6 +385,23 @@ protected function updateFieldStorageDefinitionsToRevisionable(ContentEntityType
+    $storage_definition = BaseFieldDefinition::create('boolean')
+      ->setName($field_name)
+      ->setTargetEntityTypeId($entity_type->id())
+      ->setTargetBundle(NULL)
+      ->setLabel(t('Default revision'))
+      ->setDescription(t('A flag indicating whether this was a default revision when it was saved.'))
+      ->setStorageRequired(TRUE)
+      ->setTranslatable(FALSE)
+      ->setRevisionable(TRUE);

Just add a ->setInternal(TRUE) there, for now. It's indeed related to #2933518: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this in that it's not clear how/if clients/consumers A) need to interpret this data, B) should set this field in PATCH or POST requests.

jonathan1055’s picture

In response to #105 and #106:

Thanks effulgentsia and platch, No apology needed, this core chage did not break rules, it just caused testing to fail. The Autocomplete test creates an expected set of node autocomplete values which are given to the user when selecting elements for conditions. The new 'default revision' flag appears in the results but was not in our expected set. The issue is #2936679: Autocomplete test needs values for "Default Revision" flag and it will be a simple fix for Rules.

Jonathan

timmillwood’s picture

Wim Leers’s picture

To make #109 happen, @timmillwood wrote this at #2933518-5: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this:

Can we expand this issue to mark both revision_translation_affected and revision_default as @internal?

This surely is well-intentioned, but there's one big problem: revision_translation_affected already is shipping with Drupal 8.4., but revision_default is new in 8.5. Therefore we need to treat them differently. It is critical that we do not add more fields to our HTTP API's entity representations until we're actually certain that A) we want them there, B) they have understandable semantics. It is not critical to remove something that is already there (revision_translation_affected).

So, @timmillwood, can you please create a separate critical issue to mark revision_default internal? Rather than the patch that you posted at #2933518-6: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this, which makes both revision_default (new in 8.5, API addition, which should be removed before 8.5.0, so that there is no API addition and hence no BC break) and revision_translation_affected (already exists in 8.4, which will need discussion to figure out whether A) we want it exposed via HTTP APIs, B) what its semantics are, C) if we remove it, how we handle BC). Then we can in a future non-critical issue decide how/if we want to expose revision_default.

I know this is frustrating and tricky. But just adding new data that is exposed via our HTTP API is something that must be done with the utmost consideration. This is the consequence of having a HTTP API (via the REST module) that automatically exposes things: it's nice that it does things automatically, except when it gets in the way (of iterative development), like in this issue. Thanks for your understanding!

plach’s picture

I don't think we should remove revision_default from the API representation, if that's what ::setInternal() does. See #2933518-10: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this. Wrt documentation, what kind of documentation would you find useful and what's the right place to add it?

plach’s picture

I'm not sure how adding revision_default is implying a BC break, unless you count failures in tests hardcoding a representation as BC breaks. You will have one more field returned and that's it, it can be safely ignored if you don't need it, OTOH it will be extremely useful if you need it. And since it's automatically populated by the system, if it's not part of a POST / PATCH request nothing will break, and if it's part of it it will be simply ignored. I think the only potentially problematic case is if we have a name collision, but the update combined with the example provided in the CR should allow to handle that.

plach’s picture

Issue tags: +8.5.0 release notes

Btw, I think we should mention this in release notes.

Wim Leers’s picture

Status: Fixed » Active
Issue tags: +Needs change record updates

Correct, adding a new field isn't a BC break, but if we remove it from the HTTP API after 8.5.0 is shipped, then it would be a BC break. And that's what #84 + #85 were suggesting.

it can be safely ignored if you don't need it, OTOH it will be extremely useful if you need it.

And since it's automatically populated by the system, if it's not part of a POST / PATCH request nothing will break, and if it's part of it it will be simply ignored.

From the name of the field alone it's impossible to unambiguously infer these semantics. At minimum, this should be explained in the CR.

On the PHP side, you have both \Drupal\Core\Entity\RevisionableInterface::isDefaultRevision() and \Drupal\Core\Entity\RevisionableInterface::wasDefaultRevision(). On the HTTP API side, there's only revision_default. Even as a Drupal expert, revision_default could refer to either of those methods.

It was decided years ago that the RESTful Web Services module should expose entities via HTTP APIs (and the JSON API + GraphQL modules do this too) based on the Typed Data metadata. Therefore any addition/change to an entity type's definition should also consider the impact on HTTP APIs that are automatically generated from this definition. That is my concern.

I think that ideally, the name would have been unambiguous (or at least less ambiguous), for example revision_was_default.

if it's part of it [a POST / PATCH request] it will be simply ignored.

If this is the case, we should have explicit REST test coverage for it. I see that \Drupal\KernelTests\Core\Entity\RevisionableContentEntityBaseTest::testWasDefaultRevision() does this for the PHP API. We need the same for the HTTP (REST) API. Otherwise the HTTP APIs will always be lagging.

I'm trying to perform my duty as API-First Initiative coordinator here. Part of that duty is to ensure that we really become API-First. So we need to think about our HTTP APIs, always, and especially for patches like these, where there is a direct impact. I understand this is lots of extra work, and requires a change in mindset. But otherwise Drupal 8's HTTP APIs will perennially be catching up. We recently achieved 100% REST integration test coverage for entity types. Please expand that test coverage to cover Entity API additions like these. That's why it exists: to ensure that the same can be done via HTTP APIs, and to ensure it works for all relevant entity types. (I hate to reopen issues like this, but if nobody asks these hard questions, then we just make things harder for Drupal 8 to become trulyAPI-First.

plach’s picture

I think that ideally, the name would have been unambiguous (or at least less ambiguous), for example revision_was_default.

Well, "for example" is the problem here: I'd like to see some +1s on revision_was_default before spending time on a rename.

It was decided years ago that the RESTful Web Services module should expose entities via HTTP APIs (and the JSON API + GraphQL modules do this too) based on the Typed Data metadata. Therefore any addition/change to an entity type's definition should also consider the impact on HTTP APIs that are automatically generated from this definition.

I guess I was aware about this in the back of my mind, but now that I read it and had time to parse it, I find this a bit concerning. The Entity API is not designed to use the storage as an API, this concept is even part of our BC policy. Of course field definitions provide one level of abstraction on top of raw storage, but the problem remains IMO, because this breaks encapsulation. Ideally we should expose the API as the HTTP API, although I can see how this would be a massive step back in terms of flexibility. In a way, I'm afraid we will always have this kind of clash between the "official" API and the HTTP API, which is fine, I guess, given the amount of advantages compared to drawbacks :)

Wim Leers’s picture

Ideally we should expose the API as the HTTP API

Which API is the API in this sentence?

In a way, I'm afraid we will always have this kind of clash between the "official" API and the HTTP API, which is fine, I guess, given the amount of advantages compared to drawbacks :)

Indeed! At least until a hypothetical future where we would be doing internal HTTP requests aka subrequests, which of course comes with its own set of downsides.
But given that we'll always have this clash, that means every change to Entity/Field/Typed Data API should ask the question Does this impact API-First, and if so, how?

plach’s picture

Which API is the API in this sentence?

The Entity API.

Or any other API, really. Ideally anything we provide a non-internal interface for, since in a parallel universe alternative implementations may not rely on fields ;)

catch’s picture

revision_was_default seems fine for the field name.

I didn't object to the REST API changes because if you actually want to replicate content using REST then knowing if a revision was default or pending seems useful (if the consumer is a Drupal site).

plach’s picture

I have been discussing this issue with @Wim Leers and @catch (separately).

Personally, I'm a bit uncomfortable with revision_was_default because at first sight "was" seems to exclude "is", while revision_default = 1 applies also to the actual/current default revision. OTOH "was" refers to the point in time when the revision "was" created and that's always in the past, so the name is formally correct :)

Anyway, given that the REST module does not work with revisions yet, I was wondering how an HTTP client is supposed to indicate whether a new revision should be default or pending. At the moment I believe we don't have a way to specify that in a POST request. For this reason I'm wondering whether it would make sense to use the revision_default field itself as a "setter": if it's set to 1 when POSTing a new revision, this would be created as default, while 0 would trigger the creation of a pending revision. We would achieve this by internally calling ::isDefaultRevision($revision_default) when the revision_default field is populated. This would be entirely consistent with the logic that currently populates the revision_default field on save.

To sum up, HTTP clients would rely on the revision_default field to know whether the revision they are handling was default when created, and to set its default status when creating a new one. To determine whether a revision is the current default revision, they would need to load the current default revision and compare its revision ID with the ID of the revision being handled.

If we go this way it would probably make sense to keep the revision_default field name as-is.

  • effulgentsia committed 49c6761 on 8.6.x
    Issue #2891215 by plach, timmillwood, hchonov, amateescu, catch,...

  • effulgentsia committed 9182b21 on 8.5.x
    Issue #2891215 by plach, timmillwood, hchonov, amateescu, catch,...
effulgentsia’s picture

I can think of 3 options to address #115 / #119:

  1. Leave the field named revision_default, with it only being available as a setter for new revisions, per #119.
  2. Same functionality as above, but rename the field to revision_created_as_default or similar, to be more explicit about that.
  3. Rename the field to revision_was_default, make it read-only for HTTP APIs, and introduce a new computed field for revision_is_default. Upon POSTing a new revision, allow the computed revision_is_default field to be set. This clarifies the semantics, but I don't know if there's implementation challenges with allowing a computed field to be set in some situations.

  • effulgentsia committed 14ea7b0 on 8.5.x
    Revert "Issue #2891215 by plach, timmillwood, hchonov, amateescu, catch...
effulgentsia’s picture

Status: Active » Postponed

I discussed this with @plach and @xjm, and decided to revert this from 8.5.x to remove it from the alpha until we settle on the field name. I left it in 8.6.x and am happy to re-cherry-pick it to 8.5.x after the alpha is tagged, so marking this issue postponed until then. In the meantime, I suggest perhaps starting a new issue to discuss if we want to rename the field per #122 or other suggestions.

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.

Wim Leers’s picture

#119:

Anyway, given that the REST module does not work with revisions yet, I was wondering how an HTTP client is supposed to indicate whether a new revision should be default or pending. At the moment I believe we don't have a way to specify that in a POST request.

Correct!

For this reason I'm wondering whether it would make sense to use the revision_default field itself as a "setter": if it's set to 1 when POSTing a new revision, this would be created as default, while 0 would trigger the creation of a pending revision. We would achieve this by internally calling ::isDefaultRevision($revision_default) when the revision_default field is populated. This would be entirely consistent with the logic that currently populates the revision_default field on save.

As a non-expert in the subtleties of revisions, this sounds like music to my ears!

This would mean revision_default would no longer behave like a computed field (in the committed patch you can't set it, only read it), even though it is not a computed field. It'd then behave like a "regular" field. Or are there still ways in which it would behave differently?

To sum up, HTTP clients would rely on the revision_default field to know whether the revision they are handling was default when created, and to set its default status when creating a new one.

This sounds good! But there's an edge case: what would happen when REST clients (or PHP code using the Entity API) would not specify a value? Would it then fall back to the behavior that was in the previously committed patch?

To determine whether a revision is the current default revision, they would need to load the current default revision and compare its revision ID with the ID of the revision being handled.

Sorry if this is a trivial question, but I can't find an obvious answer: how does one load the current default revision? I think the default revision is always what one gets when loading an entity using EntityStorageInterface::load()? (The only other mention of it is in \Drupal\content_moderation\ModerationInformationInterface::getDefaultRevisionId()?)

If we go this way it would probably make sense to keep the revision_default field name as-is.

+1


#122:

  1. revision_created_as_default would indeed be more explicit. One thought: it would feel strange to POST revision_created_as_default, because it's not yet been created, it's being created.
  2. Interesting proposal! I think it could be slightly simpler: revision_was_default would be the computed field, revision_is_default would be a non-computed field (since it can be set). When another revision becomes the default, the revision_is_default field value of the revision that was the default until then would have its value modified. This way, there are no special cases, no exceptional semantics.
    This would just mean that revision_is_default would not use the UniqueField constraint, but something like a yet-to-be-created UniqueRevisionField constraint: only one revision can have this set to TRUE, all other revisions must have it set to FALSE.

I don't (yet) have a strong preference for any of the three proposed solutions: #119=#122.1, #122.2 or #122.3. All three would offer clearer semantics for HTTP APIs!

Curious to see what others think!

P.S.: I'm sorry to see this reverted, but given that we already have multiple proposals that would result in clearer semantics, I think it probably was the right call.

hchonov’s picture

Has somebody mentioned forward revisions? If we post a forward revision, which is a revision newer than the default one but is not a default revision, then revision_was_default feels wrong, because this revision wasn't 'not default', but is 'not default'. Actually this applies also to the case where we are posting the current default revision, which is default, not was default.

'revision_default' is fine for me, especially given the fact that we don't enforce anyone to use that field name, but allow for it to be defined in the entity type annotation.

We have to keep in mind that we have the field 'revision_translation_affected' and haven't used any past tense form in its name.

In general I think we should not use past tense form or any verbs at all when naming fields.

P.S.: The getter method name for the field revision_translation_affected is isRevisionTranslationAffected() and not wasRevisionTranslationAffected().

When dealing with a revision then it depends on the context whether "is" or "was" is correct. When dealing with past/old revisions then everything we access is "was", but if we are dealing with the default revision or with forward revisions then everything we access is "is". I would rather leave the interpretation to the developers.

Wim Leers’s picture

Has somebody mentioned forward revisions?
[…]
In general I think we should not use past tense form or any verbs at all when naming fields.

Good call! So that would eliminate options #122.2 + #122.3, and keep only #119/#122.1. Correct? Curious to see what @effulgentsia thinks about that :)


We have to keep in mind that we have the field 'revision_translation_affected' and haven't used any past tense form in its name.

Note: That field suffers from similar problems: #2933518: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this. But in that case, I noticed too late that it was being added (added in #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state, generalized in #2896845: Provide the 'revision_translation_affected' base field by default for all revisionable and translatable entity types). When it was first added, to the Node entity type, back in August 2015, I wasn't yet a maintainer of the REST module, and Serialization/REST test coverage was so weak that not a single change to its test coverage was necessary: nobody even realized that this was affecting the normalization of Node entities!

It's only since November/December of 2017 (so, a few months ago), that REST integration test coverage is complete enough to detect (many, still not all) changes in normalizations/serializations. That's how I noticed it here: this patch was changing files in core/modules/rest/.

xjm’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Postponed » Reviewed & tested by the community
xjm’s picture

What do we need to mention about this issue in the release notes?

effulgentsia’s picture

I don't think anything for alpha, since it's been reverted in 8.5.x for now. We'll need release notes for beta and 8.5.0 though.

catch’s picture

For the release notes something like:

"The entity system now stores whether a revision is saved as default at the time it was created, so that this information can be reviewed historically. This will allow better auditing and filtering of revision history on sites that make use of draft/pending revisions."

plach’s picture

I just created #2937850: Mark revision_default as internal for REST consumers to address Wim's concerns and bring this discussion forward. Per #128, I added only #119/#122.1 as proposed solution, but I'm happy to discuss alternatives, if any.

I will post a few replies to the last comments over there.

Wim Leers’s picture

I had a 45-minute call about this with @effulgentsia on the 17th. I don't remember the details because it was at the end of a very long day, but the gist was:

  1. @effulgentsia will re-commit this as-is to 8.5.x after 8.5.0-alpha1 is tagged (which happened ~22 hours ago)
  2. He was going to file 2 or 3 follow-ups, to deal with various aspects of the concerns raised — I think #2937850 (thanks @plach) covers at least one of those. Sadly I cannot remember the details about those follow-ups. I'm sure he'll create those follow-ups shortly.
  3. We discussed all aspects: naming, computed vs not, read-only vs not, the need for a validation constraint. No decisions were made, only pros/cons of all possible decisions along all of those axes were discussed.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

All right, thanks for the feedback! I re-cherry-picked this to 8.5.x but the followups still need to be opened.

Wim Leers’s picture

WRT this affecting core REST: this also affects JSON API and GraphQL in real ways — this is now causing JSON API to have Drupal minor version-specific test coverage, because 8.4 and 8.5's data models are diverging. See #2930028-36: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

Status: Fixed » Closed (fixed)

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