Problem/Motivation

#2669802: Add a content entity form which allows to set revisions changed EntityTestRev and made it implement RevisionLogInterface, but that's wrong because we have dedicated entity types for that.

Proposed resolution

Revert the incorrect changes from #2669802: Add a content entity form which allows to set revisions .

Remaining tasks

Review, etc.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
FileSize
2.09 KB

And a patch.

Status: Needs review » Needs work

The last submitted patch, 2: 2835588.patch, failed testing.

Berdir’s picture

Removing the revision_id patch was already in the first patch, the revision log fields were added later, missed that.

This will cause a test fail I think in a test that I actually had to adjust in that issue.

Might not have been necessary to remove it, but I think there's nothing wrong with removing either as it is generated by the parent. See the very first patch in that issue.

amateescu’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

The problem with 'revision_id' being removed and revisionability of 'langcode' being changed is that it somehow breaks \Drupal\entity_test\Entity\EntityTestUpdate in #2248983-200: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table...

Also, I shouldn't edit patches manually.

Berdir’s picture

Ok, lets see what happens.

See EntityDefinitionUpdateTest in that other patch for the change that I had to make.

Somehow we have a scenario there we make an entity type revisionable but we already have a revision_id as it is hardcoded there.. so that test change kind of made sense to me, as it seems like a more realistic scenario for what would happen in such a case?

Status: Needs review » Needs work

The last submitted patch, 5: 2835588-5.patch, failed testing.

Berdir’s picture

Had a look at that test fail. And it's also in entity_test_update (adding the $entity_type_id to the fail message in UpdateTestBase helps to see that).

The problem is simply but rather problematic. In your new test, you add drupal-8.2.1.bare.standard_with_entity_test_enabled.php.gz.

Put simply, that means that your test is going to fail in the future every single time we make any change whatosever to an entity_test entity_type. Add a new one, remove a field, add a field.. anything.

Obviously that's problematic, the only way to deal with that is to either update that dump which would be very painful to do or to write update functions for entity_test. Which would obviously also be annoying.

So we could remove those changes that the original patch there added (agree with removing revision log stuff), but they are a) IMHO correct, because entity_test_update does not have a revision entity key, why on earth should it have a revision_id field? :) and b) are going to happen in like a dozen other issues that are making changes.

Suggestion:

In #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table, move EntityTestMulWithRevisionLog and EntityTestWithRevisionLog to a new test module that we use *only* for that/those tests. I think that is a pattern that we should use anyway, and have an issue to split up entity_test. With that patch, that modules has *31* entity types, most of them content entities, and enabling entity_test in a web test results in the creation of *58* tables.

hchonov’s picture

I agree with berdir and what he is suggesting however EntityTestMulWithRevisionLog is a new entity type but EntityTestWithRevisionLog is an old one. So I would create only for this test a separate module with copies of both which should never be changed.

amateescu’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
3.73 KB

So we could remove those changes that the original patch there added (agree with removing revision log stuff), but they are a) IMHO correct, because entity_test_update does not have a revision entity key, why on earth should it have a revision_id field? :) and b) are going to happen in like a dozen other issues that are making changes.

Agreed, let's keep the changes that impact EntityTestUpdate.

Also agreed with the proposal for EntityTestWithRevisionLog and EntityTestMulWithRevisionLog, but let's move that discussion back to #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
Berdir’s picture

Looks good to me as well now. Those test changes are mostly reverts of what was done in the other issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2835588-10.patch, failed testing.

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

Re-queued for testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 317c12d and pushed to 8.3.x. Thanks!

  • alexpott committed 63e73e9 on 8.3.x
    Issue #2835588 by amateescu, Berdir: Restore EntityTestRev's behavior to...

Status: Fixed » Closed (fixed)

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