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 solutionWrite a patchReviews
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.
Comment | File | Size | Author |
---|---|---|---|
#93 | entity-revision_default-2891215-93.patch | 23.34 KB | effulgentsia |
#91 | entity-revision_default-2891215-90.patch | 23.58 KB | plach |
Comments
Comment #2
timmillwoodVery early patch.
Comment #3
timmillwoodHelps if I upload the patch for the correct issue!
Comment #4
timmillwoodSome initial questions:
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.
Should this be an entity_key? Should it be not null? Would a boolean field be better?
Should this be in it's own interface?
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.
oops!
Comment #6
plach1: 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 :)
Comment #7
timmillwood@plach
isDefaultRevision($value)
is called to change that. Therefore when an entity object is created, should the field return default, until changed inisDefaultRevision()
? 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?isDefaultRevision()
method.Comment #8
hchonov1. 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.
Comment #9
timmillwood@hchonov
isDefaultRevision(False)
.Comment #10
plachTo 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 thedefault_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?
Comment #11
timmillwoodUpdated 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?
Comment #12
hchonovRe #9:
Everyone who is creating a non-default draft revision should be setting the revision type explicitly to draft, not only content moderation :).
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:
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.
Comment #13
catchI'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.
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?
Comment #14
timmillwood@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.Comment #15
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'm not sure this is correct, it will mark every revision as default when we install the new base field..
Comment #16
hchonovRe #13:
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:
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.
Comment #17
plach@hchonov:
Well, in my mind manually setting the RTA flag is not about not doing the
revision_type
change, it is about not doing therevision_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 :)
Comment #18
plach@amateescu:
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.
Comment #19
timmillwood@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.
Comment #20
plachAh, I had no idea we already supported creating drafts :)
Comment #21
hchonovRe #17:
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?
Comment #22
plachYep, sounds like a good idea :)
Comment #24
plachI'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.
Comment #25
plachHere 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.Comment #26
timmillwoodI 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"?
Comment #27
plachThis should be better
Comment #28
plachOops, missed #26:
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.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.
Comment #29
plachComment #30
phenaproximaHow about naming it pending_revision, with the corresponding method being isPendingRevision()?
Comment #31
plachThis 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.
Comment #32
plach@phenaproxima:
We did not settle on a definitive method name yet, more discussion is welcome, however we are currently going with
::wasDefaultRevision()
.Comment #33
plachHere 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.Comment #34
plachAnd now the correct interdiff...
Comment #35
plachComment #36
plachHere's the follow-up mentioned in #31: #2930295: Add a "Revision type" field to help tracking revision owners.
Comment #37
hchonovI hope this is only there for debugging purposes?
Comment #38
plachThe 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.
Comment #39
hchonovIn which case could this happen?
Comment #40
plachWell, 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.
Comment #41
timmillwoodWhy (int)? What if it's a string ID? Should we also provide the entity type ID in the exception?
Should we document that it throws a LogicException?
Why did you decide this should be a field added in EntityFieldManager rather than in ContentEntityBase?
Although this looks to be a good change to UpdatePathTestBase, it's unrelated, so maybe should go in a separate issue?
Comment #42
hchonovI 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?
Comment #43
timmillwoodI guess it would only be not set if
isDefaultRevision()
returns NULL, is that possible?Comment #44
hchonovisDefaultRevision()
always returns a boolean, so this could not happen.Comment #45
plachQuickly addressed the reviews above, everything else is still @todo.
Comment #46
plachThis 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.
Comment #47
plachThis adds the "revision_default" revision metadata flag.
Comment #48
plachCleaned-up obsolete cruft
Comment #49
plachThis should have less failures.
Comment #50
plachThis should be ready for review.
Comment #51
plachUpdated the IS
Comment #53
plachThis should fix the last failure, everything else is caused by testbots not cooperating.
Comment #54
plachComment #56
plachComment #57
gabesulliceOverall, this looks really good!
One last, minor thing...
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?
Comment #58
plachThanks, this should be better.
Comment #59
gabesulliceThanks for the fix :)
Comment #60
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe're losing the small optimization of checking the statically cached revision metadata keys here, is there any reason for that?
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.
Comment #61
plachHow 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.Comment #63
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk, 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: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?Comment #64
plachDiscussed 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 therevision_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 ;)
Comment #65
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis looks much better! Thanks to the review-bait above, I did not even open the interdiff :P
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.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 todefault_revision
to matchdefault_langcode
?Can't we just return
TRUE
when the entity is new?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" :)
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?
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?
$revision_default_affected_key
=>$revision_default_key
, but it should actually be$default_revision_key
based on the second review point.Comment #66
plachAddressed #65.
I kept
revision_default
as the field name, sincedefault_revision
may be confusing, given we have also::isDefaultRevision()
. Additionally this is consistent with the other revision metadata keys.Comment #67
plachForgot to update a couple of comments.
Comment #68
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedShouldn'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.
Stale comment, should mentioned 'revision_default' now :)
Comment #71
plachThis should fix the last failures, address #68, and add explicit test coverage for the update.
Comment #73
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedYou forgot to use
$installed_entity_type
here :)This class needs a "Test" suffix.
Comment #74
plachGeez... 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 ;)
Comment #75
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks great now!
Comment #76
hchonovI 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.
Comment #77
plachAFAICT 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.
Comment #78
plachOr maybe a faulty migration...
Comment #79
hchonovThis 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.
Comment #80
plachI 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.
Comment #81
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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 enforcerequired
. For SQL storage, I think we already do that viaNOT 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 allrequired
fields.However:
Looks to me like we're missing a
setRequired()
?Comment #82
plachSetting 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.
Comment #83
plachFixed an outdated comment.
Comment #84
effulgentsia CreditAttribution: effulgentsia at Acquia commentedHm, 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.
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 whenrevision_default
is not already TRUE on the basis that the revision ID was at one time the default regardless of later saves?Comment #85
plachI 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.
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.
Comment #86
catchThis is looking good to me now, and glad we went for the minimal API here with the option to expand it later.
Comment #87
plachCool, 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.
Comment #88
hchonovWill the entity validation fail even if a default value is set?
Comment #89
plachProbably 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.Comment #90
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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.
Comment #91
plachBrilliant, this is exactly what we needed!
Comment #92
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGlad I could help :) #91 is RTBC for me if the testbot agrees.
Comment #93
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRerolled for #2924724: Add an API to create a new revision correctly handling multilingual pending revisions.
Comment #94
plachAdded change record at https://www.drupal.org/node/2936349.
Comment #95
plachUpdated IS
Comment #96
plachComment #97
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRemoving self credit (#93 was just a straight reroll) and ticking credit boxes for reviewers.
Comment #98
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSeems 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).
Comment #99
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.6.x and cherry picked to 8.5.x.
Comment #100
plachAwesome, thank you!
Comment #101
BerdirThis 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.
Comment #102
BerdirFollow-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 :)
Comment #103
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@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.
Comment #104
timmillwood@jonathan1055 - here is the commit https://cgit.drupalcode.org/drupal/commit/?id=9182b2169722957eae5c82d326...
Comment #105
effulgentsia CreditAttribution: effulgentsia at Acquia commented@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.
Comment #106
plach@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.
Comment #107
Wim LeersThese are HTTP API additions we have to support forever if we're not careful!
#84:
#85:
… yes indeed, and I'm unpleasantly surprised that this patch was able to land, change API responses, and I was never even notified.
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.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 inPATCH
orPOST
requests.Comment #108
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedIn 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
Comment #109
timmillwoodSwitching this back to fixed, because I don't think we want to see any further patches or commits in this issue, but I agree the follow ups should be marked critical.
#2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key
#2933518: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this
#2936679: Autocomplete test needs values for "Default Revision" flag
Comment #110
Wim LeersTo 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:
This surely is well-intentioned, but there's one big problem:
revision_translation_affected
already is shipping with Drupal 8.4., butrevision_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 bothrevision_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) andrevision_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 exposerevision_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!
Comment #111
plachI 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?Comment #112
plachI'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.Comment #113
plachBtw, I think we should mention this in release notes.
Comment #114
Wim LeersCorrect, 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.
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 onlyrevision_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 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.
Comment #115
plachWell, "for example" is the problem here: I'd like to see some +1s on
revision_was_default
before spending time on a rename.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 :)
Comment #116
Wim LeersWhich API is
in this sentence?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
Comment #117
plachThe 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 ;)
Comment #118
catchrevision_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).
Comment #119
plachI 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", whilerevision_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 therevision_default
field is populated. This would be entirely consistent with the logic that currently populates therevision_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.Comment #122
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI can think of 3 options to address #115 / #119:
revision_default
, with it only being available as a setter for new revisions, per #119.revision_created_as_default
or similar, to be more explicit about that.revision_was_default
, make it read-only for HTTP APIs, and introduce a new computed field forrevision_is_default
. Upon POSTing a new revision, allow the computedrevision_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.Comment #124
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #126
Wim Leers#119:
Correct!
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?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?
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()
?)+1
#122:
revision_created_as_default
would indeed be more explicit. One thought: it would feel strange toPOST
revision_created_as_default
, because it's not yet been created, it's being created.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, therevision_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 theUniqueField
constraint, but something like a yet-to-be-createdUniqueRevisionField
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.
Comment #127
hchonovHas 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
isisRevisionTranslationAffected()
and notwasRevisionTranslationAffected()
.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.
Comment #128
Wim LeersGood 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 :)
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 ofNode
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/
.Comment #129
xjmComment #130
xjmWhat do we need to mention about this issue in the release notes?
Comment #131
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #132
catchFor 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."
Comment #133
plachI 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.
Comment #134
Wim LeersI 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:
Comment #136
Gábor HojtsyAll right, thanks for the feedback! I re-cherry-picked this to 8.5.x but the followups still need to be opened.
Comment #137
Wim LeersWRT 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.