In #2167641-38: EntityInterface::uri() should use route name and not path, @Berdir called out a couple places that we were still appending paths to the end of getSystemPath(). It was out of scope to fix there, but we can do it here now.

CommentFileSizeAuthor
#9 interdiff.txt1.39 KBtim.plunkett
#9 entity-path-2188915-9.patch4.98 KBtim.plunkett
#3 interdiff.txt1.61 KBtim.plunkett
#3 entity-path-2188915-3.patch5.26 KBtim.plunkett
#1 entity-path-2188915-1.patch4.29 KBtim.plunkett
#1 diff-no-whitespace.txt3.72 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new diff-no-whitespace.txt3.72 KB
new entity-path-2188915-1.patch4.29 KB

We need to use module_implements_alter the same as content_translation.

I attached a whitespace-ignoring patch as well, since a big chunk is just indented. config_translation_entity_info was running on all entity types, heh.

Status: Needs review » Needs work

The last submitted patch, 1: entity-path-2188915-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new entity-path-2188915-3.patch5.26 KB
new interdiff.txt1.61 KB

Oh, FieldInstance. It defines it's edit-form dynamically, so we have to do the same thing for this.

However, instead of throwing another moduleExists in there, we can just check the link templates, in case another module eventually wants to supplant config_translation...

Crell’s picture

Crell commented 
  1. +++ b/core/modules/config_translation/config_translation.module
@@ -90,16 +90,38 @@ function config_translation_theme() {
+    if ($entity_type->isSubclassOf('Drupal\Core\Config\Entity\ConfigEntityInterface')) {

    I think I missed something... WTF does that method do, and why isn't it instanceof?? 

  2. +++ b/core/modules/config_translation/config_translation.module
@@ -90,16 +90,38 @@ function config_translation_theme() {
+function config_translation_module_implements_alter(&$implementations, $hook) {
+  switch ($hook) {
+    // Move some of our hook implementations to the end of the list.
+    case 'entity_info':
+      $group = $implementations['config_translation'];
+      unset($implementations['config_translation']);
+      $implementations['config_translation'] = $group;
+      break;
   }

    The purpose of this function completely escapes me. Pretty much all uses of module_implements_alter() are a fugly hack indicating bad underlying architecture. Surely we can do better than that in core.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

4.1) So we used to have 2 different flavors of this in core:
is_subclass_of($entity_info['class'], 'Foo')
in_array(class_implements($entity_info['class']) 'Foo')

And there might have been one more.

So we now have 

class EntityType {
  public function isSubclassOf($class) {
    return is_subclass_of($this->getClass(), $class);
  }
}

4.2)
Yeah, okay, suggestions welcome. content_translation does the same thing for the same reason.

Crell’s picture

Crell commented

Point 1: Um. Um. Those two examples you list are for when we have a class name, and want to know if that class is a sublcass of/implements interface X. The method in question is operating on an object. For an object if ($foo instanceof Class) will work for both parent classes and interfaces. Different use case. $foo->isSubclassOf(), as written here, should be identical to instanceof, except instanceof pays attention to use statements.

One of us is missing something patently obvious. It's after midnight here though, so it's a tossup as to which of us it is. :-)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Let's say you want to know if a node is a ContentEntity or a ConfigEntity.
You don't have an instance of the Node class, so you cannot do $node instanceof ContentEntityInterface.

You have the entity type info. The thing that if you call $entity_type->getClass() will return \Drupal\node\Entity\Node.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

About the hook stuff, would it help to use the alter hook?

We are adding new controller classes, but we're also adding them to existing entity types, we don't define our own, so we alter those.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new entity-path-2188915-9.patch4.98 KB
new interdiff.txt1.39 KB

Good point, content_translation uses hook_entity_info_alter, and its my fault we don't use that in config_translation.
Switching that around and removing the hook_module_implements_alter works fine.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

I think this looks good. The dynamic stuff for the FieldInstance is a bit special, but necessary I think.

This will overlap with #1981858: Rename hook_entity_info/alter() to hook_entity_type_build/alter(), but shouldn't be a complicated re-roll for either issue, so I don't care which one gets in first.

Crell’s picture

Crell commented

Let's say you want to know if a node is a ContentEntity or a ConfigEntity.

Uh. In what situatoin are nodes ConfigEntities? Isn't the whole point of ConfigEntities that we can stop bastardizing nodes like that? And if you don't have an instance of the object, what are you calling -> on (as opposed to ::)?

That is still an unexplained design flaw for me at this point. Reimplementing language functionality in user space is a major code smell. If we're using that as a proxy for some other higher-level operation/inspection, then we should just outright do that and rename the method (even if the implementation happens to be similar, which I'm still not convinced of).

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

@Crell, you either did not read what I wrote, or did not actually read the code, or both. I opened #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() to just put that to bed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed f451e3d and pushed to 8.x. Thanks!

diff --git a/core/modules/config_translation/config_translation.module b/core/modules/config_translation/config_translation.module
index 4ed4784..858659a 100644
--- a/core/modules/config_translation/config_translation.module
+++ b/core/modules/config_translation/config_translation.module
@@ -96,7 +96,7 @@ function config_translation_entity_info_alter($entity_info) {
       }
       elseif ($entity_type_id == 'field_instance') {
         $class = 'Drupal\config_translation\Controller\ConfigTranslationFieldInstanceListController';
-        // Will be filled in dynamically, see \Drupal\field\Entity\FieldInstance::urlRouteParameters().
+        // Will be filled in dynamically, see \Drupal\field\Entity\FieldInstance::linkTemplates().
         $entity_type->setLinkTemplate('drupal:config-translation-overview', 'config_translation.item.overview.');
       }
       else {

Fixed during commit.

Status: Fixed » Closed (fixed)

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