getEntityTypeFromStaticClass - poor performance

I guess when we added this we said it's not going to be called very often - ouch.

Looks like the calls are coming from node_permissions_get_configured_types(), which is called from node_node_access(). Which seems to be a pretty weird feature, I don't think that's documented in config schema? Or tested?

Anyway, it would be relatively easy to add a static cache to that method, with the risk that in some very rare scenarios (one of those I just looked at today) could mess things up.

We should possibly also statically cache node_permissions_get_configured_types(), because calling NodeType::loadMultiple() 150 times isn't going to be free. especially without config entity static cache.

Can you share what exactly was profiled or provide access to the full xhprof report?

Another thing that might help a bit is removing the additional logic that we added to more explicitly deal with multiple exact matches on the entity class/type, which @fago wanted. Before, we returned on the first exact match, now we throw an exception if there's more than one, so we always have to loop over all of them.

Attaching full xhprof. Not sure if this is the same run, but it's a similar one.

We could easily throw a cache on it, but I'd love to see if we could just simplify the whole process. That's a hell of a lot of work for what it's doing.

+1 to removing node_permissions_get_configured_types(), which will kill the calls to node_type_get_types() as far as the profiled page is concerned.

I'm not sure about simplifications for the method. As said, one approach would be to remove the nice exception for multiple exact matches, which actually breaks stuff now.

Another is that we now have enough usage of those to try and go back to msonnabaum's initial classname conversion approach to see if that works. Would imply that a bunch of classes (Like Term and Vocabulary) will need to override that method, which is something we could live with to get a performance boost.

Alternatively, the bulk of the logic in Entity::getEntityTypeFromStaticClass() could be move to the entity manager (ie. something like EntityManagerInterface::getEntityTypeFromClass($classname)), where we could easily add caching.

Moving it to the entity manager sounds like an interesting idea too.

This removes that function. Unsurprisingly, doesn't look like we have test coverage for this. Maybe this should be done in a separate issue?

I had the exact same thought as #7.

I'd still prefer simplifying the process if possible, but if we can't, breaking it up and moving most of it to entity manager makes sense to me.

Something like this?

- Moved the method and added static caching.
- I also fixed a problem with the method while I was at it if you have multiple subclasses *and* an exact match that is not returned first.
- The unit tests need to be cleaned up, most of it should probably move to EntityManagerTest, only the part about passing the correct class name around needs to be kept in EntityTest. Just made them not fatal for now, easier to see if other tests have a problem with this, but I doubt that.

It is much better on the EntityManager, but this method still feels very complex. It could be broken up some, but it's unclear to me why we even need to do some of these checks.

// Check if this is the same class, throw an exception if there is more
// than one match.
if ($entity_type->getClass() == $class_name) {
  $entity_type_id = $entity_type->id();
  if ($same_class++) {
    throw new AmbiguousEntityClassException($class_name);
  }
}

What is the scenario where this would actually happen? Why is it not ok to just return once a match is found?

Also, if I'm reading this right, one invocation of this method will autoload all defined entity classes because of those is_subclass_of calls. I'd like to make sure that check is absolutely necessary before we accept that.

Also, don't we have most or all of this information from the annotation discovery anyway?

So the use case for the is_subclass_of is to be able to swap out an entity type class MyEntityType with a child class MyEntityTypePatched without breaking direct calls to MyEntityType::load() in existing code.

That really an information that we should add to the entity type discovery information.

#13: The reason this could happen is if you dynamically expose multiple entity types with the same class, see https://www.drupal.org/project/eck as an example that could do it like this in 8.x. It was not in the original implementation, but @fago was strongly for adding it, and I just wanted to get it in, so I included that. It will never work for those, but without it, it will just pick the first, while that code will force an exception.

#16: Hm, now that we have EntityType as an object for this information, we could possibly have a getOriginalClass() method and the class would ensure that the originally set class would be returned there? Yes, then we might be able to get rid of this logic..

What do you think about this approach?

Also updated the unit test coverage so that the separate methods are separately cached, removed a lot of complexity from EntityUnitTest, as we now only need to test smaller snippets.

This is certainly a big improvement. It still feels like the check for duplicates belongs elsewhere, but I can live with this if necessary.

Yeah, I'd be fine with removing that too, but @fago explicitly requested it and I didn't want to go behind his back on this one.

Fixed a issue where the subclass entities could not be loaded anymore, tested on a live environment.
Now allowing the actual class name as well, this makes sense right?

New unit test also included in the patch.

reroll

needs review

reroll needs review

Changed randomName() to randomMachineName().

Reroll

We need a change record for the removal of the configurable permission stuff (it would not work right now anyway without the schema and anything, but still) - if we want to do this here.

Yep let's get a draft record done for the removal of node_permissions_get_configured_types(). Also the issue summary seems out of date.

yeah, moving this to the EM makes sense. I also like how the patch moves to keeping the original class - that makes the detection more robust already.

This is certainly a big improvement. It still feels like the check for duplicates belongs elsewhere, but I can live with this if necessary.

Where would you put it then instead? I think we either need to be sure that the returned entity type is correct by checking as we do now, or document to require a unique entity class by entity type and rely on that. I still think that just silently returning a wrong entity type is a WTF and the wrong thing to do.

Drats!

git ac https://www.drupal.org/files/issues/entity-type-from-class-2307681-32.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32382  100 32382    0     0  63547      0 --:--:-- --:--:-- --:--:-- 65418
error: patch failed: core/lib/Drupal/Core/Entity/EntityManager.php:11
error: core/lib/Drupal/Core/Entity/EntityManager.php: patch does not apply

Committed 9d77777 and pushed to 8.0.x. Thanks!

This broke phpunit code coverage reports - see
#2324293: PHPUnit code coverage broken