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.
A couple people have WTF'd at this. It was added to stop people from guessing if they should use is_subclass_of or class_implements.
Comment | File | Size | Author |
---|---|---|---|
#81 | entity-class-implements-2194783-81-interdiff.txt | 344 bytes | Berdir |
#81 | entity-class-implements-2194783-81.patch | 38.98 KB | Berdir |
#78 | entity-class-implements-2194783-78-conflict.txt | 666 bytes | Berdir |
#78 | entity-class-implements-2194783-78.patch | 38.99 KB | Berdir |
#74 | entity-class-implements-2194783-74-interdiff.txt | 1.48 KB | Berdir |
Comments
Comment #1
tim.plunkettComment #2
amateescu CreditAttribution: amateescu commentedHow about replacing it with an easy way to tell if an entity type is content or config? That's the primary use case anyway.
Comment #3
tim.plunkettI'm open to suggestions, I'm just tired of defending usage of our own API because people don't read the methods they're critiquing.
Comment #4
Crell CreditAttribution: Crell commentedIf you can't understand the method does without reading its code then the method is misnamed.
It sounds like amateescu is right. The goal here isn't to determine if object $o is a subclass of some other object (for which instanceof is the correct way to answer that question.) The goal is to determine if the entity type definition object in question is a content or config type. Or, since we need to be mindful of Asimov's Law ("2 is the least likely number in the universe"), which variety of entity it is. There's two readily-available ways I can think of to do that:
I have no strong preference between those two.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat, or just
$entity_type->isSubTypeOf('\Drupal\Core\Entity\ContentEntityInterface')
?Comment #6
tim.plunkettThat would be s/isSubclassOf/isSubTypeOf/, which doesn't really bother me at all.
Crell, you have to realize that we do NOT have an entity object here. We have the entity type object, which we're calling ->getClass() on.
Comment #7
Crell CreditAttribution: Crell commentedTim: I do realize that. isSubclassOf() is still a misleading and wrong method, doubly so when you are passing it a class name rather than a machine name.
Comment #8
BerdirThe problem with type is that we're then talking about the type of the entity type. Too many "type"'s :)
I was think about something similar in #2119905: Provide @ConfigEntityType and @ContentEntityType to have better defaults but I don't like to use type. Subtype is also wrong, because that is already reserved because apparently want to rename bundle to subtype. And it's not a subtype, it's a more a parenttype or supertype.
A term that I was considering was "group". We also have to keep in mind that contrib will likely have other groups/types, for example entities that represent remote data from another system might be their own group that is neither content nor config.
We could also just special case the two relevant cases for core with $entity_type->isContent()/->isConfig() and contrib will have to do explicit interface checks if they want to introduce additional groups.
Comment #9
BerdirAlso, the referenced issue *would* allow you to do do $entity_type instanceof ConfigEntityType or similar, but that would make the use of those annotations mandatory, and I'm not 100% sure if we really want that. It's also not a pattern that contrib can extend because the annotation classes need to live in the same folder AFAIK. There might be a way around that, not sure.
Comment #10
sun+1
I cannot even imagine why contrib would want to introduce a new entity meta type, so that's an edge-case we do not have to support.
I double-checked the existing patch here and indeed, the only use-cases are
->isContent()
vs.->isConfig()
.Comment #11
BerdirNow that we have ConfigEntityType and ContentEntityType, and at least the first one being mandatory now due to getConfigPrefix (will soon only exist on that), it would also be pretty save to rely on that, all we'd need is an interface for Content too (there are a bunch of methods that are only relevant for it too). Then you could do if ($entity_type instanceof ConfigEntityTypeInterface).
Comment #12
msonnabaum CreditAttribution: msonnabaum commentedIgnoring the fact that we added a method to replace a core php function, this issue is treating symptoms of a bad design.
Conditional logic based on class type is one of the classic OOP code smells. Adding something like isContent and isConfig does not fix the underlying problem that we're not using polymorphism, scattering business logic outside of where it belongs.
Here are a couple examples of isSubclassOf usage to illustrate my point:
In CommentManager, not only are we putting the knowledge of "only content entities have comments" here, but also in the callers. Every caller I see checks if the return value is empty before doing something, which means this code also knows that "no fields returned" means, "this entity type doesn't have comments.
How about EntityType::isCommentable?
Also, my description of the business rules are what I could determine from the code, but I could certainly be wrong because neither the code nor the comments suggest that this rule exists. It just assumes everyone knows there's something unique about comment handling between these two types of entities. Adding a method like isCommentable makes the intent clear while also maintaining compatibility with new entity types.
Drupal\views\Plugin\views\argument_validator\Entity uses isSubclassOf before calling $this->entityManager->getFieldDefinitions($entity_type_id), to decide what to use for the bundle name. Again, this code assumes special knowledge about the difference between ConfigEntity and ContentEntity instead of calling methods that explain *why* this conditional logic exists. Here's a potential before/after:
Other examples may be more complex than this to fix, but I can assure you they are worth fixing.
In the meantime, +100 to tim.plunkett's patch as that method is absurd. If we want to do anything better than this patch, we should open followups to fix the usages.
Comment #13
BerdirAs we've been trying to explain multiple times, that is not correct. Yes, it probably was a bad idea as it *looks* like that, but what it is is a shortcut for "is_subclass_of($entity_type->getClass(), '...')", or using the old array based syntax, "is_subclass_of($entity_info['class'], '...')"
That is not possible because EntityType is a \Drupal\Core\Entity concept, while comments is a concept of an optional module.
This isn't the first time that have the feeling that you like to ignore that we are not in the lucky position to design a specific, coherent application, we're creating more or less connected building blocks that will be used in ways we can not even imagine. We can not have nice methods for every specific use case.
(changed the order a bit to give my response a better order)
We have configuration and content entities. That's not just a class or interface name/type, it is a fundamental concept of the entity system that we have two different groups of entity types. Right now, we use the is subclass of (be that by using the helper method or the to identify the group, and yes, that looks ugly. This issue suggested two different approaches, the isContent/isConfig methods or checking the entity type class/interface. #2116551: Fix the UX of Entity Reference fields is another possibility, which introduces an extendable $group property, which also allows to add more groups.
Whatever way we chose, we need a way to check if an entity type is configuration or content, or possibly even something else*. And that should be as good as possible, and we all agree that the current approach is bad, Tim's patch doesn't make it any better or worse in my opinion.
* One typical example is exposing a set of remote objects as entities, so that they can be integrated with things like rules and developers can interact with them with an API they know.
Comment #14
Crell CreditAttribution: Crell commentedI agree that we can't bake all of the possible is*Able() methods into the classes directly for the reasons Berdir mentions. That's the "other 5%" where I disagreed with Mark in his Portland Core Conversation, if he recalls. ;-) However, that doesn't mean we can't do far better than what we have now.
Even a string-key method would be better than either the current status quo or hard-coding the content/config duality. IE, EntityType::hasAttribute($key). $entity_type->hasAttribute('commentable'), etc. (Bikeshed the method names later.) That is extensible, including at runtime, but still results in code that is reasonably self-documenting.
Berdir: That Mark and I, both reasonably intelligent people, both look at isSubclassOf() and go "why are we reimplementing PHP language logic?" is IMO a sure-fire sign that it's a horribly terrible method in the first place. Doubly so if that's not what we're doing but making it look like we are. :-)
Comment #15
msonnabaum CreditAttribution: msonnabaum commentedApologies. I assumed the method did what it says it does :)
It's a fair point (although there are cases in core where we have hardcoded checks for known core modules that are important concepts), so traditional polymorphism may not work there. However, checking the class name is still a bad idea, because as you pointed out, there will likely be other entity classes that don't fit comfortably in the content/config categories. A better solution would probably be CommentManager::isEntityCommentable, that checked config managed by comment module.
What about the second example? If I'm wrong there, then sure, maybe I'm being idealistic. But if I'm right, that should explain why I continue to encourage us to write descriptive methods instead of returning "definitions", and then making decisions using arrays.
I apologize if it's annoying, but I continue to see us not even try to write better code. Writing a helper for "is_subclass_of($entity_type->getClass(), '...')" is encouraging a method that should be avoided.
Doesn't that seem like a contradiction? Are configuration and content fundamental concepts or just two entity classes that core provides? I definitely view it as the latter. They are two implementations; we don't need to start hardcoding references to them when we need different behavior. If we have the need to switch behavior, I'd argue that it probably makes more sense to do that at the entity type level rather than entity class.
Comment #16
BerdirYes, fully agreed. I'm just trying to explain that we need to be able to make that check, just removing the method doesn't solve/change anything really.
The attribute thing is an interesting idea, comment.module would then have to implement hook_entity_type_alter(), check if it's a content entity (or actually, if it's a fieldable entity ($entity->getEntityType()->isFieldable()) because as comments are attached through a field, that's the actual requirement*).
Yes, sorry I forgot about that. That example code is weird, there's simply no reason for the code to be try and do that, because we *have* a getBundleLabel() method. Which that code uses too :) I actually noticed this myself as well and opened #2204239: Simplify and de-duplicate argument validators a few days ago.
I'd like to think that we are trying. The example code that you picked there is quite interesting. Those definition arrays were converted to definition objects a while ago, with interfaces and documented methods. The code there is just a left-over that relies on the ArrayObject-based BC that we had to add as it was too much to convert at once and we haven't been able to remove it.
So assuming that code would make any sense, it should be written like this now:
But as I said, all of it can be dropped, and we can use $bundle_label that is fetched directly above ;)
*Actually: the requirement now is a fieldable content entity with an integer entity ID. As we recently made the field schema capable of supporting entities string ID's but now comment.module (and others that store entity_id as an integer) is in trouble ;)
Comment #17
Berdir1: entity-is_subclass_of-2194783-1.patch queued for re-testing.
Comment #19
BerdirWe have a ContentEntityTypeInterface now, once #2204697: Move getConfigPrefix() to ConfigEntityTypeInterface is in then I'll provide a patch for my suggestion above...
Comment #20
chx CreditAttribution: chx commentedRazing the field before planting something better is a good idea. Ie. I really like this patch, too.
Comment #21
joachim CreditAttribution: joachim commentedI filed another issue a while back; only just seen this one.
Basically, my suggestion there is: add a reasonably sanely-named method that contrib can use. Its internals may or may not be ugly, but internals can be prettied up later.
Comment #22
andypostNice, +1 to removes code fragility and drupalism
So needs to be deprecated at current stage and usage removed
Comment #23
BerdirHere's how this would look. Which shows the problem with this, most checks are actually now for FieldableEntityInterface, which is not the same. So not sure what to do.
Comment #25
andypostFixed broken test, also cleaned comment module implementations and test
Comment #26
Crell CreditAttribution: Crell at Palantir.net commentedI can't see any reason to not RTBC this right now. Other than maybe filing the follow-up to remove the deprecated method?
Comment #27
andypostit needs follow-up to remove, but I think other places should be checked like a comment module
Comment #28
BerdirAs mentioned above, this is a problem..
Since this issue started, we introduced FieldableEntityInterface as a more generic kind of entity that has fields but does not have to be a content entity.
Not sure how to deal with this.
Comment #29
tim.plunkettSo then that one can use
if (!is_subclass_of($entity_type->getClass(), '\Drupal\Core\Entity\FieldableEntityInterface')) {
Comment #30
BerdirYeah, but that one is unfortunately 50%+ of the calls, but yes, I guess that's what we need to do :)
Comment #31
AntonnaviHere is patch entity-is_subclass_of-2194783-25.patch updated with changes from comment #29
Comment #34
Crell CreditAttribution: Crell as a volunteer commentedIt looks like #31 isn't fully accounting for #28 and following. It's sometimes replacing FieldableEntityInterface with ContentEntityInterface, other times not. I don't know the Entity interface family well enough to know which ones are correct and which aren't. Berdir, fago?
Comment #35
andypostOn one hand we need to get rid of method that actually a helper and thus should not be a part
EntityType
interface, so logic said move it to trait...On other hand removal makes DX worst because developer always needs to understand which what he wants from entity type:
1) is entity fieldable (extendable) - check that real *entity class* implements
FieldableEntityInterface
2) is entity a content - check annotation class for
ContentEntityTypeInterface
3) is entity a config - check annotation class for
ConfigEntityTypeInterface
So maybe better to add
isFieldable
method toContentEntityTypeInterface
?Otoh if we add new behaviours to content or config entities we will need to implement new methods to check it? Example a related issue #2409677: add a method to EntityTypeInterface to determine whether an entity type is content or config
Comment #36
Mile23Comment #38
andypostComment #39
vprocessor CreditAttribution: vprocessor at Skilld commentedrerolled
Comment #41
vprocessor CreditAttribution: vprocessor at Skilld commentedfixed
Comment #42
andypostshould be 9.0.0
Comment #43
Crell CreditAttribution: Crell as a volunteer commentedThis can use a class constant for better clarity.
Comment #44
vprocessor CreditAttribution: vprocessor at Skilld commentedfixed
Comment #45
andypostthis is no go
EntityChangedInterface::class
the php 5.5 feature
Comment #46
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #47
dawehnerThis also changed the logic, before it checked for the fieldable interface .. also note: isSubclassOf checked the actual entity class, not the entity type class
Comment #48
vprocessor CreditAttribution: vprocessor at Skilld commentedfixed
Comment #50
andypostnit, unneeded change
looks something missed
wrong conversion
missed to change
Comment #51
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #52
vprocessor CreditAttribution: vprocessor at Skilld commentedfixed
Comment #53
Mile23"@deprecated in Drupal 8.2.x, will be removed before Drupal 9.0.0. Use is_subclass_of() instead."
Comment #54
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #55
vprocessor CreditAttribution: vprocessor at Skilld commentedfixed
Comment #56
Mile23Still needs to wrap at 80. :-)
Comment #57
vprocessor CreditAttribution: vprocessor at Skilld commented>>Still needs to wrap at 80. :-)
What do you mean Mile23?
Comment #58
Mile23As with all docblocks, this comment should wrap at 80 characters.
Sorry I didn't specify it earlier, but it's our standard: https://www.drupal.org/coding-standards/docs#drupal
Comment #59
andypostFixed #53 to common pattern user in the file
Also fixed the rest of usage
comment module to use check for
FieldableEntityInterface
consistently, maybe move that to separate issue?Suppose for config entities better to convert to instanceof
Comment #61
andypostFix test
Comment #62
andypostif everyone agree on approach, only comment module changes is question
Comment #63
larowlanyeah this is out of scope but happy with it
Comment #64
Mile23Patch in #61 applies.
Still needs change record.
Re-running the testbot.
Comment #66
BerdirSo, @catch in #2789315-82: Create EntityPublishedInterface and use for Node and Comment proved (again) that just deprecating this and using is_subclass_of() is also confusing for reviewers.
Previously, I suggested we should use the entity type annotation class. But we only have content and config, and we're adding a whole bunch of additional interfaces, traits and base classes and we need a solution for that too.
I just had the idea that maybe the only problem here is the method name. What if we simply rename this to entityClassImplements() or something like this and deprecate the old method name in favor of that? Better name suggestions are also very welcome, that's just the first thing I thought of.
Comment #67
catchYes that gets me every single time :(
entityClassImplements() isn't a bad idea.
Comment #68
BerdirLets try that then. Completely new patch, so no interdiff.
Took the liberty to also update old class strings with their ::class equivalent, I think that better shows how this is supposed to look now.
Comment #70
BerdirComment #72
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks perfect! I agree that the new method name makes it much more clear what it's about :)
Comment #73
catchSo this all looks good except we have $entity here so could just check instanceof directly?
Comment #74
BerdirYeah we should. Found another instance, also changed interface, both must have been added at the same time.
Comment #75
jibranBack to RTBC as #73 is addressed.
Comment #76
joachim CreditAttribution: joachim as a volunteer commented> How about replacing it with an easy way to tell if an entity type is content or config? That's the primary use case anyway.
If that is the purpose of this method, then there is nearly something better than this: both ConfigEntityType and ContentEntityType annotations have a property $group, which is 'config' and 'content' respectively.
It just needs an accessor.
Comment #77
BerdirNeeds a reroll.
See #66, that's no longer the only use case, we have a whole bunch of additional entity interfaces and we'll add more. We'll have more and more checks that an entity type supports a certain feature/implements a certain interface.
Comment #78
BerdirConflicted in ContenEntityForm.
Comment #79
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedStill looking great.
Comment #80
alexpottWas going to commit this but we still need a CR.
Need to add a new line here
Comment #81
BerdirChange record: https://www.drupal.org/node/2842808
Added the missing newline.
Comment #83
catchCommitted/pushed to 8.3.x, thanks!