The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this

Why isn't it really a field?

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

On a semi-related note, #2674518: Add revision log field to views is looking to expose revision metadata fields to Views, does marking things internal also hide them from Views?

This will need an upgrade path and tests, but let's see what testbot thinks of it.

A brief note, I will post a more complete comment later, if needed.

The new revision_default field is not supposed to be populated (POST/PATCH), but it may definitely be useful to have it available in the response, as it's going to be a critical part of pending revision translation and likely many other workflow / workspaces use cases. There is no new documentation around this field because the API seemed to be self-documenting to me:

A flag indicating whether this was a default revision when it was saved.

Not sure what's so mysterious about it.

The case of revision_translation_affected is slightly different: it is populated automatically by the system, but only if a value is not explicitly set by the API consumer. Hence it's a regular field, which users may find it useful to read and write: for this reason I'm not sure it's correct to mark it as internal. I was hoping that the documentation we added lately was enough to clarify the meaning of this field.

ok, as @Wim Leers suggested, let's keep this issue for revision_translation_affected only for now.

In this patch I have removed setting revision_default to internal, and updated block_content, node, and media resource tests.

We'd still need an upgrade path to make existing revision_translation_affected fields internal, and decide if removing this field from serialized entities is a BC break or not, and how to get around that.

it is populated automatically by the system, but only if a value is not explicitly set by the API consumer

Can you give a reason why someone may want to set a value for this externally? Since it's very crucial to set this correctly to ensure the integrity of the content model I think it may indeed make sense to forbid setting this field externally. At the very least we should restrict it to people with elevated access.

Do we have a way to prevent write and allow read? Because if we don't, I'd say we have to bite the bullet and keep it around, as the RTA flag can be critical to know in multilingual scenarios.

Aside from that, I'm not sure it's correct to prevent write just because we can't come up with a reason, right now. I wound't be able to mention one a couple of years ago and now I had the need to set it explicitly in #2924724: Add an API to create a new revision correctly handling multilingual pending revisions.

I think most people just need to know that they can safely ignore this field unless they know what they are doing.

Do we have a way to prevent write and allow read?

Well since Rest checks access on a field-level, we could prevent edit access on this field in the access control handler. Since this field never shows up in forms this should have no other consequences.

I think most people just need to know that they can safely ignore this field unless they know what they are doing.

I disagree. You can completely botch up your content by e.g. setting this to FALSE where it shouldn't be. For example, revisions will be hidden in the UI with no way to recover them (without introspecting the DB). The reverse problem is also confusing, although it's not as bad: If people set this to TRUE where it shouldn't be, content editors will see revisions with no differences which is annoying if you have some sort of publishing workflow that and you want to see what changed, e.g. with the Diff module. So granting anyone who has access to fix a typo in a node title via Rest full access to this field is problematic. I realize that this is the status quo, but that doesn't mean we shouldn't fix it.

Of course you can break your content if you use it the wrong way (which means not ignoring it, btw ;), I was not questioning that. The problem I was bringing up was with the reduced flexibility. I guess we normally prefer to allow people to do stuff at their own risk rather the opposite. That said, if we go the access control handler way, it should be alterable, right? This would be enough to address my concerns.

Yes, return AccessResult::allowed() from a hook_entity_field_access() implementation should be able to override that. Although would be nice to check with someone who actually understands field access logic and/or test it.

we normally prefer to allow people to do stuff at their own risk rather the opposite

So the problem I see with that is - like I hinted at in #14 - with Rest "their own" is no longer really true. I.e. any content editor of a decoupled site can write this field which is not the case with normal Form API. I agree that a custom module should still be able to set this field value, but I think we can still do the access thing, as that module can then simply choose to ignore access-checking on that field.

I think we can still do the access thing, as that module can then simply choose to ignore access-checking on that field.

+1

If an entity sync is implemented through REST, then I guess that both revision_translation_affected and revision_default should be exposed. How could this be achieved if they are flagged as internal?

If an entity sync is implemented through REST

@hchonov, that's a very good point. Not trying to put more gas on this fire, but this is (or was?) the main purpose of the REST module :)

Re #19: You are talking about GETting entities, though, not POST/PATCHing them, right?

@amateescu, this makes me think of that we need something like a setting on each entity resource whether internal fields should be exposed, so that we could define an entity resource for entity sync including all fields and also an entity resource for retrieving entities for some other use case where the internal fields should be excluded.

@tstoeckler, I am talking about GETing and POST/PATCHing. If you are able to retrieve an internal field through REST, then you should be able to include that field when POST/PATCHing an entity through REST.

We are on the way to start implementing some sort of content syncing and were considering to use REST and at first we want to be able to sync all kind of fields without restrictions - I am pretty sure that even if some fields are flagged somehow read-only or whatever we'll end up needing a solution for overwriting this.

Do I understand it correctly that it "might" be the wrong decision to consider REST at the moment for implementing entity sync?

As further evidence for the need to resolve this issue, we noticed that as we switched our site from D8.x to D9.x the kernel tests for our REST modules were breaking for the first time. So we wrote a script to find out what was happening under the covers. The script created a node and then in ten iterations of a loop created for each of two languages first an unpublished, then a published revision. Then we loaded each of the revisions and for each of the revision each of the translations and reported whether the revision was published for the language, whether the translation was affected for the revision, and whether it was the default revision. Running it under Drupal 8 if a revision was created for a given language, isRevisionTranslationAffected() returned TRUE and it returned FALSE for the other language (when the language was present, which of course it wasn't for the second language in the very first revision).

The Drupal 8.9 documentation for isRevisionTranslationAffected() says:

Checks whether the current translation is affected by the current revision.

Then we ran the same script under Drupal 9 and isRevisionTranslationAffected() returned TRUE only for the very last revision, and for BOTH languages in that revision.

So we looked at the Drupal 9 documentation for the method. It says:

Checks whether the current translation is affected by the current revision.

So I think a reasonable person would expect that if the behavior of the method is going to change, there would some corresponding change in the documentation, and maybe even some elaboration about what "is affected" means, exactly.