Problem/Motivation

1) It makes sense to have a separate interface for ContentEntityTypes

Proposed resolution

1) Create the interface
2) Make ContentEntityType implement the interface
3) Replace usages throughout core

Remaining tasks

1) Create the patch
1) Review the patch
2) Commit the patch

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mauzeh’s picture

Starting this now.

mauzeh’s picture

mauzeh’s picture

Status: Active » Needs review
mauzeh’s picture

ConfigEntityTypeInterface now extends EntityTypeInterface, forgot to commit something locally before creating the patch...

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks great. You have mastered the Drupal coding standards, so there really isn't much to complain about. :-)
I checked and there isn't any code that checks for ContentEntityType currently, so nothing else needs to be updated.
I will introduce lots of that code in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities however, soon. :-)
In a follow-up we could discuss moving the get*Table() methods onto this interface as they really do not make sense for config entities.

However, I just remembered @plach wants to get rid of those completely from the annotation, so let's just do this for now. Baby steps :-)

Thanks @mauzeh !!!

tstoeckler’s picture

tstoeckler’s picture

FileSize
1.02 KB

Wow, I just realized that d.o borked up the file status. I've had that before and it's really strange. I totally did not delete the patch file, especially as I don't have permissions to do that on d.o.

Re-uploading the same patch unchanged.

fago’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/ContentEntityTypeInterface.php
@@ -0,0 +1,14 @@
+interface ContentEntityTypeInterface extends EntityTypeInterface {

What's the point in having an interface for a class that's no where used? Also, do have other annotation classes have an interface? I don't think so.

tstoeckler’s picture

Well we're adding one for config entities in #2204697: Move getConfigPrefix() to ConfigEntityTypeInterface, so this is just consistent.

We're going to use this heavily in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities I just didn't want to bloat that issue with a whole bunch of stuff that could be separated out as well.

Also we're checking for ContentEntityInterface a whole bunch in core so it just seems consistent to be able to perform that check without a concrete entity at hand, as well.

Lastly we're going to want to move a couple methods there. At least isTranslatable() and isRevisionable() (per #2224549: Simplify checking whether an entity type is revisionable) as those don't make sense for ConfigEntities. I just thought it was reasonable to approach this step by step.

@fago Does that suffice as an explanation?

fago’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
529 bytes

I see - that makes sense. I was confused as it looked like the patch puts the interface on the annotation class, but it turns out its just the comment of the class which is plain wrong.

So here is an updated patch which fixes the comment of the class also.

fago’s picture

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Ahh, I see. I had overlooked that. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6fe11ac and pushed to 8.x. Thanks!

The empty interface is odd but #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities is going to use it.

  • Commit 6fe11ac on 8.x by alexpott:
    Issue #2226437 by fago, tstoeckler, mauzeh: Create...
tstoeckler’s picture

Status: Fixed » Needs work

Patch #2 was committed here instead of patch #10. Please revert and re-commit as patch #2 missed the extends EntityTypeInterface part. This breaks unit testing with ContentEntityTypeInterface. Alternatively I can also upload a diff, but I think the revert should work.

tstoeckler’s picture

Title: Create ContentEntityTypeInterface » [wrong patch committed] Create ContentEntityTypeInterface
Status: Needs work » Reviewed & tested by the community

Actually, should still be RTBC, then.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: d8_content_entity_type.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.09 KB

Ok, rolling as a patch then. This is literally just a local git revert and then a git apply of #11, so marking RTBC straightaway.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5476dca and pushed to 8.x. Thanks!

Sorry for the mistake

  • Commit 5476dca on 8.x by alexpott:
    Issue #2226437 followup by tstoeckler: [wrong patch committed] Create...
tstoeckler’s picture

Title: [wrong patch committed] Create ContentEntityTypeInterface » Create ContentEntityTypeInterface

Awesome thanks!

Status: Fixed » Closed (fixed)

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