Discovered by Berdir at #2789315-52: Create EntityPublishedInterface and use for Node and Comment → and subsequent comments.
#2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it →
introduced this bug. But before describing what bug it introduced, let's explain what that issue was trying to achieve: it wanted to make it possible to PATCH comment entities at all. For the entity denormalizer to succeed, it needs to know the bundle. So, requests must include the comment type. After denormalizing the entity, one must denormalize the fields in the entity. Before denormalizing a field, field access is checked. But modifying an entity's bundle is not allowed. End result: sending the bundle was necessary in step 1, but forbidden in step 2. Hence PATCHing comment entities was impossible.
To solve this, #2631774 needed to disable field access checking for at least the bundle field: as long as it was being sent unchanged, it should be allowed. But Field API does not have a facility for detecting changes made to fields, so some logic had to be added at the REST level to manually check certain fields. But which fields?
To not hardcode this, we tried to find what aspect determined which fields are safe to send, but should match the current value. There already was one case of this: the langcode entity key field. bundle is another key. Therefore it seemed safe to make the jump to assuming that all entity keys are in fact safe to treat this way. After all, their name indicates that they are "keying the entity", i.e. together determine a unique entity. This impression is supported by the fact that:
langcode key was already being special-cased in a similar way\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema() which ensures entity keys are NOT NULL, hence seemingly confirming this is the right way to use it — being fixed in
#2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field →
nowSo this solution was committed.
This worked well for a while: #2631774 was committed on May 31, 2016. It shipped with Drupal 8.2. Nobody complained for a long time.
Then the Workflow Initiative happened, and
#2789315: Create EntityPublishedInterface and use for Node and Comment →
happened for that initiative, in November 2016. That was adding status to the entity keys. Which is an entity key that does not uniquely identify an entity. It can be modified. This then caused test failures, which I was called in to comment on:
#2789315-53: Create EntityPublishedInterface and use for Node and Comment →
. Turns out "entity keys" only exist for "fast access" purposes, or at least that's what they exist for today!
So "entity keys" are misleadingly named — created an issue for that: #2885411: "Entity keys" aren't actually keys — rename to avoid misinterpretation → .
None.
None.
None.
Fixed
8.5 ⚰️
Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.
Not all content is available!
It's likely this issue predates Contrib.social: some issue and comment data are missing.
core/modules/jsonapi/tests/src/Functional/MediaTest.php:79 actually references this issue. It was foretold there to remove the line below in this issue, but that didn't happen.
Creating a child issue to see if we can safely remove that line.