Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#18 | 2226437-18-content-entity-type-interface.patch | 1.09 KB | tstoeckler |
Comments
Comment #1
mauzeh CreditAttribution: mauzeh commentedStarting this now.
Comment #2
mauzeh CreditAttribution: mauzeh commentedComment #3
mauzeh CreditAttribution: mauzeh commentedComment #4
mauzeh CreditAttribution: mauzeh commentedConfigEntityTypeInterface
now extendsEntityTypeInterface
, forgot to commit something locally before creating the patch...Comment #5
tstoecklerLooks 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 !!!
Comment #6
tstoecklerComment #7
tstoecklerWow, 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.
Comment #8
fagoWhat'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.
Comment #9
tstoecklerWell 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?
Comment #10
fagoI 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.
Comment #11
fagoan -> a
Comment #12
tstoecklerAhh, I see. I had overlooked that. Thanks!
Comment #13
alexpottCommitted 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.
Comment #15
tstoecklerPatch #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.Comment #16
tstoecklerActually, should still be RTBC, then.
Comment #18
tstoecklerOk, rolling as a patch then. This is literally just a local git revert and then a git apply of #11, so marking RTBC straightaway.
Comment #19
alexpottCommitted 5476dca and pushed to 8.x. Thanks!
Sorry for the mistake
Comment #21
tstoecklerAwesome thanks!