Add a FilterFormatRepository providing methods to load filter formats

It is great to do those things, but I kinda doubt this will be able to land in 8.0.x

1. +++ b/core/modules/filter/src/FilterFormatRepositoryInterface.php
   @@ -0,0 +1,113 @@
   +  /**
   +   * Gets only those formats that are allowed for this user account.
   +   *
   

   Define what means allowed

2. +++ b/core/modules/filter/src/FilterFormatRepositoryInterface.php
   @@ -0,0 +1,113 @@
   +   * Gets a list of text formats that are allowed for a given role.
   ...
   +   * Returns the default text format for a particular user.
   

   Any reason for gets vs. returns?

3. +++ b/core/modules/filter/tests/src/Unit/FilterFormatRepositoryTest.php
   @@ -0,0 +1,309 @@
   +class FilterFormatRepositoryTest extends UnitTestCase {
   

   Did you considered mocking using prophecy? You should see the error messages created by prophecy, its from another universe

4. +++ b/core/modules/filter/tests/src/Unit/FilterFormatRepositoryTest.php
   @@ -0,0 +1,309 @@
   +    $entity_type_definition = $this->getMock('\Drupal\Core\Entity\EntityTypeInterface');
   

   I would argue that entity type definitions are value objects and so don't need to be mocked, as they don't consists of logic for themselves

+++ b/core/modules/filter/tests/src/Unit/FilterFormatRepositoryTest.php
@@ -0,0 +1,274 @@
+    $current_language = $this->prophesize('\Drupal\Core\Language\LanguageInterface');

What about using ::class and get autocompletion?

Sometimes along this lines ...

Really like the Interface::class pattern. :-)

I don't suppose there's an easy way to do a test-only patch here.

+++ b/core/modules/filter/filter.module
@@ -85,174 +85,69 @@ function filter_theme() {
+ * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0. Use
+ *   either \Drupal\filter\FilterFormatRepositoryInterface::getAllFormats() or
+ *   \Drupal\filter\FilterFormatRepositoryInterface::getFormatsForAccount().

The patch in #11 still applies, but there's no FilterFormatRepositoryInterface anymore.

Hey, you know what? git diff --name-only won't tell you files *added* by the patch. :-)

My proxy-fu is no good, so I can't review that. Coding standards:

+++ b/core/modules/filter/src/FilterFormatRepositoryInterface.php
@@ -0,0 +1,113 @@
+   *
+   * @return \Drupal\filter\FilterFormatInterface
+   */
+  public function getDefaultFormat(AccountInterface $account = NULL);
+

Needs a return comment.

Added CR. Rerolled, refreshed the code, added proper deprecations.

Adding #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() as related because this remove usages of drupal_static() and drupal_static_reset().

Replaced all usages of deprecated functions.

The patch from #25 was wrong. Republishing.

Ouch, an accidental edit.

::getDefaultFormat() was not properly used.

More fixes.

Fixing coding standards.

Last fix.

+++ b/core/modules/filter/filter.services.yml
@@ -9,3 +9,8 @@ services:
+  filter.filter_format_repository:

why not just filter.format_repository?

I would say, better filter.format_repository. Because the service ID is always namespaced under the module’s name, which in this case is filter.*

EDIT: @andypost, initially I misread your comment. In fact we're on the same page.

Fixed #38. Improved the service.

Added deprecation test. Fixed unit test failure.

Coding standards.

1. +++ b/core/modules/comment/src/Plugin/migrate/source/d6/Comment.php
   @@ -85,7 +85,7 @@ public function fields() {
   -      'format' => $this->t('The {filter_formats}.format of the comment body.'),
   +      'format' => $this->t('The {filter_format}.format of the comment body.'),
   
   +++ b/core/modules/comment/src/Plugin/migrate/source/d7/Comment.php
   @@ -79,7 +79,7 @@ public function fields() {
   -      'format' => $this->t('The {filter_formats}.format of the comment body.'),
   +      'format' => $this->t('The {filter_format}.format of the comment body.'),
   

   Not related & wrong changes. Undo.

2. +++ b/core/modules/filter/filter.module
   @@ -90,50 +89,36 @@ function filter_theme() {
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the
   ...
   +  @trigger_error('filter_formats() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getAllFormats() to get all formats or with ::getFormatsForAccount() to get all formats that a user is able to access. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   ...
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the
   ...
   +  @trigger_error('filter_formats_reset() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::resetFormats() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -144,15 +129,15 @@ function filter_formats_reset() {
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the
   ...
   +  @trigger_error('filter_get_roles_by_format() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the \Drupal\filter\FilterFormatInterface::getRoles() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -164,16 +149,15 @@ function filter_get_roles_by_format(FilterFormatInterface $format) {
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the
   ...
   +  @trigger_error('filter_get_formats_by_role() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getFormatsByRole() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -199,17 +183,14 @@ function filter_get_formats_by_role($rid) {
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the
   ...
   +  @trigger_error('filter_default_format() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getDefaultFormat() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -238,17 +219,16 @@ function filter_default_format(AccountInterface $account = NULL) {
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the
   ...
   +  @trigger_error('filter_fallback_format() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getFallbackFormatId() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   

   Increase version: 8.7.0 -> 8.8.0. Update also the CR.

3. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -409,6 +410,19 @@ public function removeFilter($instance_id) {
   +    // Handle the fallback format upfront (all roles have access to this format).
   

   Exceeds 80 chars.

4. +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -48,12 +55,15 @@ class FilterFormatListBuilder extends DraggableListBuilder {
   +   * @param \Drupal\filter\FilterFormatRepositoryInterface $format_repository
   +   *   The filter format repository service.
   ...
   +  public function __construct(EntityTypeInterface $entity_type, EntityStorageInterface $storage, ConfigFactoryInterface $config_factory, MessengerInterface $messenger, FilterFormatRepositoryInterface $format_repository) {
   ...
   +    $this->formatRepository = $format_repository;
   

   For BC reasons $format_repository should be optional and should trigger deprecation error if not passed.

5. +++ b/core/modules/filter/tests/src/Functional/FilterAdminTest.php
   @@ -187,8 +187,9 @@ public function testFilterAdmin() {
   +    $format_repository = $this->container->get('filter.format_repository');
   
   +++ b/core/modules/filter/tests/src/Functional/FilterDefaultFormatTest.php
   @@ -50,12 +50,13 @@ public function testDefaultTextFormats() {
   +    $format_repository = $this->container->get('filter.format_repository');
   
   @@ -65,14 +66,14 @@ public function testDefaultTextFormats() {
   +    $this->container->get('filter.format_repository')->resetFormats();
   
   +++ b/core/modules/filter/tests/src/Functional/FilterFormatAccessTest.php
   @@ -118,9 +118,10 @@ protected function setUp() {
   +    $format_repository = $this->container->get('filter.format_repository');
   
   @@ -189,17 +190,18 @@ public function testFormatRoles() {
   +    $format_repository = $this->container->get('filter.format_repository');
   
   @@ -283,8 +285,10 @@ public function testFormatWidgetPermissions() {
   +    $format_repository = $this->container->get('filter.format_repository');
   
   @@ -296,7 +300,7 @@ public function testFormatWidgetPermissions() {
   +    foreach ($this->container->get('filter.format_repository')->getAllFormats() as $format) {
   
   @@ -328,7 +332,7 @@ public function testFormatWidgetPermissions() {
   +    $this->container->get('filter.format_repository')->resetFormats();
   
   +++ b/core/modules/filter/tests/src/Functional/FilterNoFormatTest.php
   @@ -31,7 +31,7 @@ public function testCheckMarkupNoFormat() {
   +    $this->assertEqual(check_markup($text), check_markup($text, $this->container->get('filter.format_repository')->getFallbackFormatId()), 'Text with no format is filtered the same as text in the fallback format.');
   
   +++ b/core/modules/filter/tests/src/Kernel/FilterAPITest.php
   @@ -486,7 +486,7 @@ public function testDependencyRemoval() {
   +    \Drupal::service('filter.format_repository')->resetFormats();
   
   +++ b/core/modules/filter/tests/src/Kernel/FilterCrudTest.php
   @@ -66,8 +66,8 @@ public function testTextFormatCrud() {
   +    $formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/filter/tests/src/Kernel/FilterSettingsTest.php
   @@ -47,7 +47,7 @@ public function testFilterDefaults() {
   +    $this->container->get('filter.format_repository')->resetFormats();
   
   +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -64,7 +64,7 @@ protected function setUp() {
   +          'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/Functional/NodeRevisionsAllTest.php
   @@ -95,7 +95,7 @@ protected function createNodeRevision(NodeInterface $node) {
   +      'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/Functional/NodeRevisionsTest.php
   @@ -105,7 +105,7 @@ protected function setUp() {
   +        'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
   @@ -99,7 +99,7 @@ public function testNodeRevisionDoubleEscapeFix() {
   +      'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/Functional/Views/FilterNodeAccessTest.php
   @@ -57,7 +57,7 @@ protected function setUp($import_test_views = TRUE) {
   +              'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/Functional/Views/FrontPageTest.php
   @@ -283,7 +283,7 @@ protected function doTestFrontPageViewCacheTags($do_assert_views_caches) {
   +            'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/Functional/Views/PathPluginTest.php
   @@ -48,7 +48,7 @@ protected function setUp($import_test_views = TRUE) {
   +              'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/Functional/Views/RowPluginTest.php
   @@ -49,7 +49,7 @@ protected function setUp($import_test_views = TRUE) {
   +              'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/node/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -56,7 +56,7 @@ protected function setUp() {
   +        'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/quickedit/src/Tests/QuickEditAutocompleteTermTest.php
   @@ -198,7 +198,7 @@ public function testAutocompleteQuickEdit() {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php
   @@ -632,9 +632,10 @@ public function testFieldapiField() {
   +    $format_repository = $this->container->get('filter.format_repository');
   
   +++ b/core/modules/search/tests/src/Functional/SearchDateIntervalTest.php
   @@ -34,7 +34,7 @@ protected function setUp() {
   +    $default_format = $this->container->get('filter.format_repository')->getDefaultFormat()->id();
   
   +++ b/core/modules/search/tests/src/Functional/SearchLanguageTest.php
   @@ -45,7 +45,7 @@ protected function setUp() {
   +    $default_format = $this->container->get('filter.format_repository')->getDefaultFormat()->id();
   
   +++ b/core/modules/search/tests/src/Functional/SearchMultilingualEntityTest.php
   @@ -62,7 +62,7 @@ protected function setUp() {
   +    $default_format = $this->container->get('filter.format_repository')->getDefaultFormat()->id();
   
   +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -127,7 +127,7 @@ public function testBreadCrumbs() {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/taxonomy/src/Tests/TaxonomyTestTrait.php
   @@ -45,7 +45,7 @@ public function createVocabulary() {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTestTrait.php
   @@ -40,7 +40,7 @@ public function createVocabulary() {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyFieldFilterTest.php
   @@ -160,7 +160,7 @@ protected function assertPageCounts($path, $counts, $message) {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyTestBase.php
   @@ -137,7 +137,7 @@ protected function mockStandardInstall() {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/taxonomy/tests/src/Kernel/Views/TaxonomyTestBase.php
   @@ -166,7 +166,7 @@ protected function mockStandardInstall() {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/text/tests/src/Functional/TextFieldTest.php
   @@ -177,7 +177,7 @@ public function _testTextfieldWidgetsFormatted($field_type, $widget_type) {
   +    foreach ($this->container->get('filter.format_repository')->getAllFormats() as $format) {
   
   @@ -216,7 +216,7 @@ public function _testTextfieldWidgetsFormatted($field_type, $widget_type) {
   +    $this->container->get('filter.format_repository')->resetFormats();
   
   +++ b/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php
   @@ -70,7 +70,7 @@ public function testTextSummaryBehavior() {
   +          'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/views/tests/src/Functional/DefaultViewsTest.php
   @@ -152,7 +152,7 @@ public function testDefaultViews() {
   +    $filter_formats = $this->container->get('filter.format_repository')->getAllFormats();
   
   +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayFeedTest.php
   @@ -49,7 +49,7 @@ public function testFeedOutput() {
   +          'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   @@ -107,7 +107,7 @@ public function testFeedFieldOutput() {
   +          'format' => $this->container->get('filter.format_repository')->getDefaultFormat()->id(),
   
   +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
   @@ -283,7 +283,7 @@ public function testTextInputRequired() {
   +    $display['display_options']['exposed_form']['options']['text_input_required_format'] = $this->container->get('filter.format_repository')->getDefaultFormat()->id();
   
   +++ b/core/modules/views/tests/src/Kernel/Handler/AreaTextTest.php
   @@ -55,7 +55,7 @@ public function testAreaText() {
   +    $view->display_handler->handlers['header']['area']->options['content']['format'] = $this->container->get('filter.format_repository')->getDefaultFormat()->id();
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
   @@ -907,7 +907,7 @@ public function testEntityReferenceComputedField() {
   +    $entity->field_test_text->format = $this->container->get('filter.format_repository')->getDefaultFormat()->id();
   

   Tests should use \Drupal::service(...) instead of $this->container->get(...).

Apart from the inline review, we should handle the case when a 3rd party code uses calls the cache reset directly drupal_cache_reset('filter_formats').

Fixed #46.

+++ b/core/modules/filter/src/FilterFormatRepository.php
@@ -0,0 +1,174 @@
+      $cid = 'filter_formats:' . $this->languageManager->getCurrentLanguage()->getId();
+      if ($cache = $this->cacheBackend->get($cid)) {
+        $this->formatsAll = $cache->data;
...
+  public function resetFormats() {
+    unset($this->formatsAll);
+    $this->formatsByUser = [];

btw Entities are could be statically cached in storage handler and I'm sure better to relay on its caching instead of internal property
Also it needs __sleep() method to unset this properties on serialize

btw Entities are could be statically cached in storage handler and I'm sure better to relay on its caching instead of internal property

Not sure I get this.

I mean kind of it, we already using static cache for user roles, so caching of formats also makes sense
I see no reason to abuse cache with already cached config entities (only sorting of formats could be moved to storage handler)

A bit of clean-up

+++ b/core/modules/filter/src/FilterFormatRepository.php
@@ -0,0 +1,136 @@
+    $formats = $this->storage->loadByProperties(['status' => TRUE]);

I see your point, but this is a performance regression compared to #47, as ::loadByProperties() is doing a real query against the DB on each ::getAllFormats() call. So, it's not only sorting of formats that occurs every time.

Just checked \Drupal\Core\Entity\EntityStorageBase::loadByProperties() and worried as well(

Then I think we should always use loadMultiple() & filter/sort in repository method but makes sense to test & I sure there should not be big diff in performance

Then I think we should always use loadMultiple() & filter/sort ...

Yep, I fully agree. better than having another caching layer.

... but makes sense to test & I sure there should not be big diff in performance

Given that there are only few text formats, usually fewer than 10 I don't know if it make sense to benchmark.

1. +++ b/core/includes/bootstrap.inc
   @@ -950,6 +950,12 @@ function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
   +      @trigger_error("Using drupal_static_reset() with 'filter_formats' as parameter is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use \\Drupal\\filter\\FilterFormatRepositoryInterface::resetFormats() instead. See https://www.drupal.org/node/3035368.", E_USER_DEPRECATED);
   
   +++ b/core/modules/filter/filter.module
   @@ -90,50 +89,36 @@ function filter_theme() {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the
   + *   'filter.format_repository' service with ::getAllFormats() to get all
   + *   formats or with ::getFormatsForAccount() to get all formats that a user is
   + *   able to access.
   ...
   +  @trigger_error('filter_formats() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getAllFormats() to get all formats or with ::getFormatsForAccount() to get all formats that a user is able to access. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   ...
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the
   + *   'filter.format_repository' service with ::resetFormats() method.
   ...
   +  @trigger_error('filter_formats_reset() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::resetFormats() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -144,15 +129,15 @@ function filter_formats_reset() {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the
   + *   \Drupal\filter\FilterFormatInterface::getRoles() method.
   ...
   +  @trigger_error('filter_get_roles_by_format() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the \Drupal\filter\FilterFormatInterface::getRoles() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -164,16 +149,15 @@ function filter_get_roles_by_format(FilterFormatInterface $format) {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the
   + *   'filter.format_repository' service with ::getFormatsByRole() method.
   ...
   +  @trigger_error('filter_get_formats_by_role() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getFormatsByRole() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -199,17 +183,14 @@ function filter_get_formats_by_role($rid) {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the
   + *   'filter.format_repository' service with ::getDefaultFormat() method.
   ...
   +  @trigger_error('filter_default_format() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getDefaultFormat() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   @@ -238,17 +219,16 @@ function filter_default_format(AccountInterface $account = NULL) {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the
   + *   'filter.format_repository' service with ::getFallbackFormatId()
   + *   method.
   ...
   +  @trigger_error('filter_fallback_format() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use the "filter.format_repository" service with ::getFallbackFormatId() method. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   
   +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -48,12 +55,19 @@ class FilterFormatListBuilder extends DraggableListBuilder {
   +      @trigger_error('The $format_repository parameter will become mandatory before Drupal 9.0.0. See https://www.drupal.org/node/3035368.', E_USER_DEPRECATED);
   

   Use the standard deprecation messages.

2. +++ b/core/modules/filter/src/FilterFormatRepository.php
   @@ -0,0 +1,136 @@
   +    $this->storage = $entity_type_manager->getStorage('filter_format');
   

   We should not do this in constructor. I hit an edge case, see https://www.drupal.org/project/drupal/issues/2863986#comment-12899598. Let's revert this

I also noticed that some tests are doing a useless cache clearing.

The failing test still needs some investigation.

Sorry, the patch from #56 was wrong. Here's the correct version. The interdiff is correct.

Reroll for 9.1.x and trying to fix test cases.

Fixing PHP lint error in #61

Fixing test cases.

Fixing failed test cases. Also regarding #54, loadbyproperties() has been used in several repository methods in core.

1. +++ b/core/modules/filter/src/FilterFormatRepository.php
   @@ -0,0 +1,141 @@
   +   * {@inheritdoc}
   ...
   +  public function getAllFormats() {
   +    $formats = $this->storage->loadByProperties(['status' => TRUE]);
   +    uasort($formats, [$this->storage->getEntityType()->getClass(), 'sort']);
   +    return $formats;
   

   We seem to be losing both static and persistent caching here.

   I kinda get the persistent cache, but note that loadByProperties() is *not* cached and is pretty slow. We could add a lookup_keys for status to FilterFormat as well, that would help a bit, but it's still a database query. And that requires an upgrade path too. I don't think removing the caching here is a good idea.

   I see that this was discussed before, but even a loadMultiple() is still an extra query. Every time. See \Drupal\Core\Config\Entity\ConfigEntityStorage::doLoadMultiple(). We cache the list of all config entities of a given type.

2. +++ b/core/modules/filter/src/FilterFormatRepository.php
   @@ -0,0 +1,141 @@
   +   */
   +  public function resetFormats() {
   +    $this->entityTypeManager->getStorage('filter_format')->resetCache();
   +    $this->formatsByUser = [];
   

   this on the other hand seems pointless. All actual changes to the filter formats already save them and imply invalidating the entity cache. And any change that does not change the format but e.g. changes user/role permissions, this is pointless.

3. +++ b/core/modules/filter/src/Plugin/DataType/FilterFormat.php
   @@ -46,7 +46,7 @@ public function getSettableOptions(AccountInterface $account = NULL) {
        // @todo: Avoid calling functions but move to injected dependencies.
        return array_map(function ($format) {
          return $format->label();
   -    }, filter_formats($account));
   +    }, \Drupal::service('filter.format_repository')->getFormatsForAccount($account));
   

   the array map thing happens quite often, we could add a method for that.

4. +++ b/core/modules/filter/src/ProxyClass/FilterFormatRepository.php
   @@ -0,0 +1,120 @@
   +<?php
   +// @codingStandardsIgnoreFile
   +
   +/**
   + * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\filter\FilterFormatRepository' "core/modules/filter/src".
   + */
   +
   +namespace Drupal\filter\ProxyClass {
   +
   

   interesting, where are we injecting it in the critical path but might not actually use it? The proxy actually adds overhead and it's only worth doing if we are saving calls. What you want to do instead IMHO is make sure that you don't do any extra work in the constructor, specifically instantiating the entity storage.

5. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayFeedTest.php
   +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayFeedTest.php
   @@ -58,7 +58,7 @@ public function testFeedOutput() {
   
   @@ -58,7 +58,7 @@ public function testFeedOutput() {
          'body' => [
            0 => [
              'value' => 'A paragraph',
   -          'format' => filter_default_format(),
   +          'format' => \Drupal::service('filter.format_repository')->getDefaultFormat()->id(),
            ],
   

   this seems like an old method that no longer exists like this, used quite a lot.

Restarted by converting #66 into a MR. I didn't make any changes, just verbatim conversion to begin with

• Updated the CR
• Updated the IS
• Addressed remarks from #69
• As we are back to static and persistent caching I'm also removing the "needs profiling" tag

This is ready for review

Applied @berdir's suggestion

Another review posted. I'm open to discuss this and get feedback from others about things like the deprecation version. And know that the service name would be a very tedious change to update due to uses and so on.

Addressed the remarks. Back to "Needs review"

Updated IS & CR

Hopefully really the last review iteration...

Thank you. I've addressed the latest remarks.

there's also this second plugin here.

Yes, I saw that before but finally I forgot to change.

Fixed latest change requests. Ready for review again

Thanks, I think this is ready.

IMHO the introduction of the repository service makes sense here, the methods are not trivial due to caching and so on. In regards to performance, I was wondering a bit, because I don't think any of these calls really are in the critical path, but I think it makes sense to essentially keep the level of caching that we have here for now, the we don't need to discuss that further. What's really neat is that the introduction of cache tags for that caches means that we can remove a lot of explicit cache invalidation and it makes it much easier to remove the caching if we consider it to no longer be necessary in the future.

As a follow-up, I'd like to explore to add a general persistent cache for loadMultiple() without ids for config entities, it's a fairly common thing for them and would likely allow us to remove the custom caching here for this. Possibly combined with the idea in #3551948: Document that EntityStorageInterface::loadMultiple may bypass caching, but likely cache first, refactoring and deprecations later. That certainly would allow us to drop this extra cache, because we essentially add a cache for the wildcard config query + the format config entity load, which is in fast chained config cache, which would be faster than this.

We did DI where we could with BC, most changes are in tests.

Just postponed #2917817 on this: would be better to land this first.

Since RTBC, all commits were rerolls. MR !14343 is the one that counts

Some minor comments on the MR.

I think it'd also be useful to have unit(?) tests to confirm that the caching in getAllFormats() and getFormatsForAccount() works and is invalidated as expected.

Looks like the CR needs updating as well:

• update the repository service ID
• Add removal version for the filter* functions
• Add note about constructor changes
• Add note about the new method in FilterFormatInterface

Update the CR according to #94

Addressed remarks except the testing of cache. I wonder if we still need that, in very rare cases we're doing that

A couple of minor comments, I'm not sure how strongly @godotislate feels about the unit tests.

If there's existing coverage, even implicit, for the caching behavior, then I think the tests aren't that necessary.

Ready for a new review

> Ok, there's a lot of back and forth with these versions, each reviewer has different POV. My head is exploding

I think we're consistently on 11.4 for 13 now, which seems fine. not sure if we'd need to bother with the BC for drupal_static_reset() until D13, it's such a super edge case, but it also doesn't hurt to keep it.

> If there's existing coverage, even implicit, for the caching behavior, then I think the tests aren't that necessary.

We definitely have test coverage for the persistent caching invalidation, otherwise plenty of tests would fail that add new formats and then use them.

getFormatsForAccount() might not be explicitly covered, but I'm not even sure we need that memory caching there. This isn't in the critical path, I had a look at how often it's called, it's 2-3 times when editing a node (multiplied by number of text fields with a format). The only thing this really caches is getting the memory-cached filters and the array_filter() with the access check, which also is cached.

But we kind of thought that we want to avoid the discussion and decision on whether or not we need it and just keep it. Fairly limited complexity I think.

On other side, I just realized that we have a bug in the caching logic, see MR review. So maybe we should have more explicit test coverage, but with my suggested refactoring, there won't be much left to test and the problem can't really happen anymore.

Status:

• Changed according the latest @berdir's suggestion. I hope I've correctly understood the proposed solution
• Updated the CR

Ready for review

Thanks, yes, exactly like that. That is much easier.

I added a note to the CR that the reset calls are very likely not needed and can probably be removed.

I'll leave it to @godotislate to decide on whether or not the test coverage here is sufficient, I think it is ok now with the updated implementation. I'd rather open a follow-up to decide if we want to remove caching from getFormatsForAccount().

Straight reroll after #3580662: Deprecate _filter_tips() has been merged

Thanks to everyone for their incredible patience to get this one over the line!

There were two minor merge conflicts in filter.module and FilterThemeHooks.php when backporting to 11.x, but these just involved the use statements. I manually resolved the conflicts.

Committed d65f017 and pushed to main and committed f7f8afd and pushed to 11.x. Thanks!