CommentManager::getAllFields() should have a static cache

Here's a failing test to get you started, your challenge, should you choose to accept it - is to make this patch green.

The potential issue with this patch is if EntityManagerInterface::clearCachedFieldDefinitions() is called during a request, it doesn't result in cache clear for CommentManager::getAllFields().

Looks good to me

So what if we added a reset method and then some timely hooks to fire to call the reset on update/delete/insert of field_config entities of type comment?

Or on EntityManagerInterface have getFieldMapByType() method or getFieldMap($type) or something like that, and have EntityManager cache the filtered fieldMap for CommentManager and when fieldMap cache is cleared with clearCachedFieldDefinitions() is also clears the filtered fieldMap.

Patch using EntityManager to handle the caching, so its refreshed when fieldMap is.

Makes sense! Let's just move CM method to EM!

Suppose issue should change title and summary.

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -52,6 +52,16 @@ class CommentManager implements CommentManagerInterface {
   +  protected $fieldMap = array();
   

   remains?

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -87,17 +97,7 @@ public function getFields($entity_type_id) {
      public function getAllFields() {
   ...
   -    return $comment_fields;
   +    return $this->entityManager->getFieldMapByFieldType('comment');
   

   there's no sense to handle this method once EM does the same

3. +++ b/core/modules/comment/tests/Drupal/comment/Tests/CommentManagerTest.php
   @@ -0,0 +1,64 @@
   +class CommentManagerTest extends UnitTestCase {
   

   this should test new entity manager method

New drupal contributor here, Mike Goulding, I'm at the Drupalcon Austin sprint. I noticed this needs to be re-rolled and I'm going to take care of that now.

Rerolled patch from comment #10.

This has once again shown up as a performance problem in HEAD. If anyone is available to give this a final push, you will be kissed by the moon and stars.

Working on it.

I started back from last green (#10). I don't fully understand what is asked in #11.

Let's remove unused method now from comment manager

$this->discovery does not exist.

Please ignore #20.
$this->discovery is an attribute inherited in the parent class, and theorically is correctly initialized from constructor.
I cannot find what is wrong there.

@penyaskito I think we need to extend existing EM unit test, no reason to test CM because EM gets new method

@andypost But the failure comes from BreakpointMediaQueryTest :-S

Fixed CM test, still needs to extend core/modules/comment/tests/src/CommentManagerTest.php

Increasing priority due to performance

have no time now, the issue needs extend entity manager test to cover new method

Yeah i'm been too busy with other things to come back to this. So all that is left is to add tests to EntityManagerTest ?

+1 on the approach, never really liked CM::getAllFields() :-).

Just a minor note that the phpdoc for the new EM property is a bit weird.

Also, there's probably some code in file.module that could leverage that new feature ?

Is this the way to go for the test? I'm not sure if I understood correctly.

@penyaskito yes, it is!
Also needs to check #35

Added test at #36 as proper patch. Small addition of some asserts.
Changed the phpdoc as @yched suggested in #34, not sure if this is more convincing.

Checked file.module for extending the usage of the new methods in there as @yched suggests in #35, but couldn't find any case.

Changed phpdoc in EntityManagerInterface.
Changed CommentManagerTest for mocking getFieldMapByFieldType().
Reused getFieldMapByFieldType() in Vocabulary::postSave().

Oops, fixed indentation.

@Berdir is right.

Nice find. Committed/pushed to 8.x, thanks!

Kissed by the moon and the stars, thanks!