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.

CommentFileSizeAuthor
#81 entity-class-implements-2194783-81-interdiff.txt344 bytesBerdir
#81 entity-class-implements-2194783-81.patch38.98 KBBerdir
#78 entity-class-implements-2194783-78-conflict.txt666 bytesBerdir
#78 entity-class-implements-2194783-78.patch38.99 KBBerdir
#74 entity-class-implements-2194783-74-interdiff.txt1.48 KBBerdir
#74 entity-class-implements-2194783-74.patch38.97 KBBerdir
#70 entity-class-implements-2194783-70-interdiff.txt947 bytesBerdir
#70 entity-class-implements-2194783-70.patch39.04 KBBerdir
#68 entity-class-implements-2194783-68.patch38.12 KBBerdir
#61 2194783-entity-is_subclass_of-61.patch37.41 KBandypost
#61 interdiff.txt839 bytesandypost
#59 2194783-54.patch37.12 KBandypost
#59 interdiff.txt28.59 KBandypost
#52 entity-is_subclass_of-2194783-52.patch12.38 KBvprocessor
#52 entity-is_subclass_of-2194783-52.interdiff.txt2.93 KBvprocessor
#48 entity-is_subclass_of-2194783-48.patch13.71 KBvprocessor
#48 entity-is_subclass_of-2194783-48.interdiff.txt4.41 KBvprocessor
#44 entity-is_subclass_of-2194783-44.patch16.1 KBvprocessor
#44 entity-is_subclass_of-2194783-44.interdiff.txt1.52 KBvprocessor
#41 entity-is_subclass_of-2194783-41.patch15.81 KBvprocessor
#39 entity-is_subclass_of-2194783-39.patch76.91 KBvprocessor
#31 interdiff.txt784 bytesAntonnavi
#31 entity-is_subclass_of-2194783-26.patch21.18 KBAntonnavi
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,020 pass(es). View
#25 interdiff.txt5.88 KBandypost
#25 entity-is_subclass_of-2194783-25.patch21.57 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,948 pass(es). View
#23 entity-is_subclass_of-2194783-23.patch18.75 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,941 pass(es), 11 fail(s), and 0 exception(s). View
#1 entity-is_subclass_of-2194783-1.patch11.17 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-is_subclass_of-2194783-1.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
11.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-is_subclass_of-2194783-1.patch. Unable to apply patch. See the log in the details link for more information. View
amateescu’s picture

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.

tim.plunkett’s picture

I'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.

Crell’s picture

If 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:

if ($type->getType() == 'content') { ... } // getType() may return "content", "configuration", or potentially some other value if someone makes a 3rd variety of entity.

if ($type->isType('content')) { ... } // Returns true or false if it matches the type the object thinks it represents.

I have no strong preference between those two.

Damien Tournoud’s picture

That, or just $entity_type->isSubTypeOf('\Drupal\Core\Entity\ContentEntityInterface')?

tim.plunkett’s picture

That 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.

Crell’s picture

Issue tags: +DrupalWTF

Tim: 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.

Berdir’s picture

The 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.

Berdir’s picture

Also, 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.

sun’s picture

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

+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().

Berdir’s picture

Now 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).

msonnabaum’s picture

Ignoring 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:


if ($entity_type->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')) {
  $fields = $this->entityManager->getFieldDefinitions($entity_type_id);
}
$bundle_name = (empty($fields) || empty($fields[$bundle_type]['label'])) ?
  t('bundles') : $fields[$bundle_type]['label'];


$bundle_name = (!$entity_type->isBundlable() || empty($bundle_label = $entity_type->getBundleLabel()) ?
  t('bundles') : $bundle_label;

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.

Berdir’s picture

Ignoring the fact that we added a method to replace a core php function, this issue is treating symptoms of a bad design.

As 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'], '...')"

How about EntityType::isCommentable?

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.

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.

(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.

Crell’s picture

I 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. :-)

msonnabaum’s picture

As 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'], '...')"

Apologies. I assumed the method did what it says it does :)

That is not possible because EntityType is a \Drupal\Core\Entity concept, while comments is a concept of an optional module.

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.

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.

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.

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.

Whatever way we chose, we need a way to check if an entity type is configuration or content, or possibly even something else

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.

Berdir’s picture

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. :-)

Yes, 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*).

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.

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 apologize if it's annoying, but I continue to see us not even try to write better code.

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:

if ($entity_type->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')) {
  $fields = $this->entityManager->getFieldDefinitions($entity_type_id);
}
$bundle_name = empty($fields) ? t('bundles') : $fields[$bundle_type]->getLabel();

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 ;)

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 1: entity-is_subclass_of-2194783-1.patch, failed testing.

Berdir’s picture

We have a ContentEntityTypeInterface now, once #2204697: Move getConfigPrefix() to ConfigEntityTypeInterface is in then I'll provide a patch for my suggestion above...

chx’s picture

Issue tags: +Needs reroll, +Novice

Razing the field before planting something better is a good idea. Ie. I really like this patch, too.

joachim’s picture

I 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.

andypost’s picture

Nice, +1 to removes code fragility and drupalism
So needs to be deprecated at current stage and usage removed

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -Novice
FileSize
18.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,941 pass(es), 11 fail(s), and 0 exception(s). View

Here'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.

Status: Needs review » Needs work

The last submitted patch, 23: entity-is_subclass_of-2194783-23.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
21.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,948 pass(es). View
5.88 KB

Fixed broken test, also cleaned comment module implementations and test

Crell’s picture

I can't see any reason to not RTBC this right now. Other than maybe filing the follow-up to remove the deprecated method?

andypost’s picture

it needs follow-up to remove, but I think other places should be checked like a comment module

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -406,7 +406,7 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
     // Fail with an exception for non-fieldable entity types.
-    if (!$entity_type->isSubclassOf('\Drupal\Core\Entity\FieldableEntityInterface')) {
+    if (!($entity_type instanceof ContentEntityTypeInterface)) {
       throw new \LogicException(SafeMarkup::format('Getting the base fields is not supported for entity type @type.', array('@type' => $entity_type->getLabel())));
     }

As 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.

tim.plunkett’s picture

So then that one can use

if (!is_subclass_of($entity_type->getClass(), '\Drupal\Core\Entity\FieldableEntityInterface')) {

Berdir’s picture

Yeah, but that one is unfortunately 50%+ of the calls, but yes, I guess that's what we need to do :)

Antonnavi’s picture

FileSize
21.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,020 pass(es). View
784 bytes

Here is patch entity-is_subclass_of-2194783-25.patch updated with changes from comment #29

Status: Needs review » Needs work

The last submitted patch, 31: entity-is_subclass_of-2194783-26.patch, failed testing.

Crell’s picture

It 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?

andypost’s picture

On 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 to ContentEntityTypeInterface?

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

Mile23’s picture

Title: Remove EntityTypeInterface::isSubclassOf() » Mark EntityTypeInterface::isSubclassOf() as deprecated for Drupal 9.0.x
Version: 8.0.x-dev » 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Issue tags: +Needs reroll
vprocessor’s picture

Status: Needs work » Needs review
FileSize
76.91 KB

rerolled

Status: Needs review » Needs work

The last submitted patch, 39: entity-is_subclass_of-2194783-39.patch, failed testing.

vprocessor’s picture

Status: Needs work » Needs review
FileSize
15.81 KB

fixed

andypost’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -359,6 +359,8 @@ public function setAccessClass($class);
+   * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 8.0.0

should be 9.0.0

Crell’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -301,7 +301,7 @@ public function __construct($definition) {
+    if (is_subclass_of($this->getClass(), 'Drupal\Core\Entity\EntityChangedInterface')) {

This can use a class constant for better clarity.

vprocessor’s picture

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -14,6 +14,11 @@
    +  const ENTITY_TYPE_ENTITY_CHANGED_INTERFACE = 'Drupal\Core\Entity\EntityChangedInterface';
    

    this is no go

  2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -301,7 +306,7 @@ public function __construct($definition) {
    +    if (is_subclass_of($this->getClass(), self::ENTITY_TYPE_ENTITY_CHANGED_INTERFACE)) {
    

    EntityChangedInterface::class
    the php 5.5 feature

vprocessor’s picture

Assigned: Unassigned » vprocessor
Status: Needs review » Needs work
dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -122,7 +122,7 @@ public function __construct(array $values, $entity_type) {
-    if (!$this->entityManager()->getDefinition($values['targetEntityType'])->isSubclassOf('\Drupal\Core\Entity\FieldableEntityInterface')) {
+    if (!$this->entityManager()->getDefinition($values['targetEntityType']) instanceof ContentEntityTypeInterface) {

This also changed the logic, before it checked for the fieldable interface .. also note: isSubclassOf checked the actual entity class, not the entity type class

vprocessor’s picture

Assigned: vprocessor » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.41 KB
13.71 KB

fixed

Status: Needs review » Needs work

The last submitted patch, 48: entity-is_subclass_of-2194783-48.patch, failed testing.

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -13,7 +13,7 @@
    -
    +  ¶
    

    nit, unneeded change

  2. +++ b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
    @@ -2,6 +2,7 @@
    +use Drupal\Core\Entity\ContentEntityTypeInterface;
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
    @@ -4,6 +4,7 @@
    +use Drupal\Core\Entity\ContentEntityTypeInterface;
    

    looks something missed

  3. +++ b/core/modules/comment/comment.views.inc
    @@ -20,16 +20,15 @@ function comment_views_data_alter(&$data) {
    -  // Provide a integration for each entity type except comment.
    +  // Provide a integration for each entity type with fields except comment.
    ...
    -    if ($entity_type_id == 'comment' || !$entity_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface') || !$entity_type->getBaseTable()) {
    +    if ($entity_type_id == 'comment' || !$entity_type->getBaseTable()) {
    

    wrong conversion

  4. +++ b/core/modules/comment/src/CommentManager.php
    @@ -100,6 +101,7 @@ public function __construct(EntityManagerInterface $entity_manager, QueryFactory
         if (!$entity_type->isSubclassOf('\Drupal\Core\Entity\FieldableEntityInterface')) {
    +      // Early return, only content types can carry comment fields.
    

    missed to change

vprocessor’s picture

Assigned: Unassigned » vprocessor
vprocessor’s picture

Assigned: vprocessor » Unassigned
Status: Needs work » Needs review
FileSize
2.93 KB
12.38 KB

fixed

Mile23’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -359,6 +359,8 @@ public function setAccessClass($class);
+   * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0

"@deprecated in Drupal 8.2.x, will be removed before Drupal 9.0.0. Use is_subclass_of() instead."

vprocessor’s picture

Assigned: Unassigned » vprocessor
vprocessor’s picture

Assigned: vprocessor » Unassigned
Status: Needs work » Needs review
FileSize
650 bytes
12.41 KB

fixed

Mile23’s picture

Status: Needs review » Needs work

Still needs to wrap at 80. :-)

vprocessor’s picture

Status: Needs work » Needs review

>>Still needs to wrap at 80. :-)
What do you mean Mile23?

Mile23’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -359,6 +359,8 @@ public function setAccessClass($class);
+   *
+   * @deprecated in Drupal 8.2.x, will be removed before Drupal 9.0.0. Use is_subclass_of() instead.

As 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

andypost’s picture

Fixed #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?

+++ b/core/lib/Drupal/Core/ParamConverter/AdminPathConfigEntityConverter.php
@@ -65,7 +66,7 @@ public function convert($value, $definition, $name, array $defaults) {
-      if (!$entity_type->isSubclassOf('\Drupal\Core\Config\Entity\ConfigEntityInterface')) {
+      if (!$entity_type instanceof ConfigEntityTypeInterface) {

Suppose for config entities better to convert to instanceof

Status: Needs review » Needs work

The last submitted patch, 59: 2194783-54.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
839 bytes
37.41 KB

Fix test

andypost’s picture

if everyone agree on approach, only comment module changes is question

larowlan’s picture

+++ b/core/modules/comment/comment.views.inc
@@ -20,16 +22,15 @@ function comment_views_data_alter(&$data) {
-    $fields = \Drupal::service('comment.manager')->getFields($entity_type_id);
-    $base_table = $entity_type->getDataTable() ?: $entity_type->getBaseTable();
-    $args = array('@entity_type' => $entity_type_id);
 
-    if ($fields) {
+    if ($fields = \Drupal::service('comment.manager')->getFields($entity_type_id)) {
+      $base_table = $entity_type->getDataTable() ?: $entity_type->getBaseTable();
+      $args = array('@entity_type' => $entity_type_id);

yeah this is out of scope but happy with it

Mile23’s picture

Status: Needs review » Needs work

Patch in #61 applies.

Still needs change record.

Re-running the testbot.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

So, @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.

catch’s picture

Yes that gets me every single time :(

entityClassImplements() isn't a bad idea.

Berdir’s picture

Title: Mark EntityTypeInterface::isSubclassOf() as deprecated for Drupal 9.0.x » Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements()
Status: Needs work » Needs review
FileSize
38.12 KB

Lets 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.

Status: Needs review » Needs work

The last submitted patch, 68: entity-class-implements-2194783-68.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 70: entity-class-implements-2194783-70.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Looks perfect! I agree that the new method name makes it much more clear what it's about :)

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -282,7 +282,7 @@ public function updateFormLangcode($entity_type_id, EntityInterface $entity, arr
+    if ($entity->getEntityType()->entityClassImplements(EntityChangedInterface::class)) {

So this all looks good except we have $entity here so could just check instanceof directly?

Berdir’s picture

Yeah we should. Found another instance, also changed interface, both must have been added at the same time.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #73 is addressed.

joachim’s picture

> 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.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Needs 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.

Berdir’s picture

Conflicted in ContenEntityForm.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Still looking great.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Was going to commit this but we still need a CR.

diff --git a/core/modules/comment/comment.views.inc b/core/modules/comment/comment.views.inc
index 1c00130..fb11a4f 100644
--- a/core/modules/comment/comment.views.inc
+++ b/core/modules/comment/comment.views.inc
@@ -4,6 +4,7 @@
  * @file
  * Provide views data for comment.module.
  */
+
 use Drupal\Core\Entity\ContentEntityInterface;
 
 /**

Need to add a new line here

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
38.98 KB
344 bytes

Change record: https://www.drupal.org/node/2842808

Added the missing newline.

  • catch committed 3d72d4b on 8.3.x
    Issue #2194783 by Berdir, vprocessor, andypost, Antonnavi, tim.plunkett...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Committed/pushed to 8.3.x, thanks!

Status: Fixed » Closed (fixed)

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