Content Translation should use EntityPublishedInterface or the 'published' entity key to determine if it needs to add its 'content_translation_status' field

Thanks @amateescu for doing what I was too lazy for :)

The main challenge here is how we deal with the impact this has on existing content entities like menu_link_content that have a published key that does *not* point to status. If we'd just switch the key now then what would happen is that suddenly the content_translation_status field would no longer be used and instead, it would switch to enabled for that.

I just learned today that content_translation is a bit special in that regard in that it has special logic to keep an already defined storage definition, even if isn't needed anymore, although I'm not 100% sure right now if this would also cover this.

Idea that I just had that isn't fully thought through yet: We add a temporary flag for entity types that makes content_translation use the keys, something like content_translation_respect_keys = true. Only if that is TRUE, it respects the keys. In 9.x, we switch the default of that key to TRUE, in 8.x, if the key is not set and the keys do not match the current hardcoded value, we do a @trigger_error().

The same could be done for uid/author btw.

Added patch for this. This should be acceptable, as the concern raised by Berdir it's handled because we check if the field is translatable in \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus, and menu_link_content is making that false.

After applying patch in #8, I get the following error when I visit the translation page of a menu item:

Error: Call to undefined method Drupal\content_translation\ContentTranslationMetadataWrapper::getEntityType() in Drupal\content_translation\ContentTranslationMetadataWrapper->isPublished() (line 90 of core/modules/content_translation/src/ContentTranslationMetadataWrapper.php).

I thought it made sense changing:

$this->getEntityType()

to:

$this->translation->getEntityType()

Because $this->translation should be of type EntityInterface (that's what the constructor says). After this change, the error does not happen anymore.

Patch attached.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This will need a test case to show the issue.

I stumbled over this also from a performance perspective.

isPublished() is called a lot from content_translation_language_fallback_candidates_entity_view_alter() if you are displaying many entities, for example nodes in a view or embedded things like paragraphs or also menu link content entities, this is currently quite slow.

We have EntityPublishedInterface now, which is optimized to use the entity key value system which allows to return this value without instantiating a field object.

Created a merge request from this and extend the code to also check for EntityPublishedInterface. Unfortunately, this doesn't actually work for menu_link_content, the biggest offender for performance in our case, exactly because of the published/status situation. Just changing this class to respect that isn't enough, we also would need to change \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus(), but that's where things get complicated. If we just fix that, then content_translation will switch over to the enabled field and stop using its own field which might have data in it. I don't really know how to handle that. We would probably need to write an update function, loop over all enabled entity types that have no status field but have a published entity key and then migrate the data over, assuming translatability of the field is set correctly.

Note that the wrapper currently also blindly assumes that the entities it wraps have a getOwner() and a getChangedTime() method, without checking for the respective interfaces. There's also #2850400: Content Translation does not use EntityOwnerInterface properly which touches the owner topic.

Changing this to a task because I don't think it's really a bug, the wrapper can be subclassed, if it doesn't work for you you can customize it, it's a performance optimization/code generalization so it works for more cases out of the box. On the plus side, I think it doesn't really need tests for the current changes. It will need tests if we go all the way for the menu_link_content implications for example.

Doesn't appear to be a data model change, hiding older patches too.

Change appears to match the issue summary and tests are appearing green.

Would agree with @berdir around the need for tests, believe an existing test would fail if the change caused disruption. If anything maybe a follow up for extending coverage but wouldn't say a blocker here.

LGTM

Leaving at this status, but per #15, I don't think this is really done.

This is only half the picture, the other half is \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus(), which still has the hardcoded status.

That means the only effective change here is that entities that *do* have a status field will now use isPublished(), which makes this check likely a bit faster (didn't profile yet).

But entities that don't have a status field but have a published entity like like MenuLinkContent are not yet affected at all. As written, changing them would result in an immediate field structure change that is hard to deal with.

Should we move back for profiling?

If I understand #15 and #18 then followups are needed. One concerning \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus(), one about menu_link_content and another for getChangedTime(). The last one will be similar to #2850400: Content Translation does not use EntityOwnerInterface properly. I have not checked for duplicates.

The changes in the MR seem correct to me. That leaves tests. In #15 Berdir argues that tests are not needed. But this does introduce logic that I am not sure if it is implicitly tested elsewhere.

Changing back to needs work for now.

There are two changes now here.

1) The original change to use the published entity key. Without also changing \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus(), this does nothing. Any entity that does not have a status field will automatically get the content_translation_status field, and it will never look for status or a published field. That also means this can't be tested with a kernel or functional test since that code path doesn't exist. A unit test would be able to do it, but at that point we're faking up a scenario that's not real. Therefore it is IMHO not useful to make this change in isolation.

2) The other change is using EntityPublishedInterface, which is a performance optimization. Nothing is broken, it's just isn't as fast as it could be. So we can't have a failing kernel or functional test again. We could wire up a unit test and explicitly assert that it's calling isPublished() when the passed in object implements that interface, but I'm not sure if it's worth doing that.

At the same time, 2) isn't as good as it could be without having as many cases as possible go through that check, so getting that in on its own isn't *that* useful.

> one about menu_link_content

That can't be a separate follow-up either, because it's directly tied to the hasPublicationStatus() logic. If we change that then menu_link_content falls apart. We could customize its handler and wrapper classes to not do that, but we still will break contrib and cutom entity types in the same way, so we need a solution for that anyway.

These content_translation issues are really hard to untangle...