Refactor SiteSettingsLoader class [#3176345]

Problem/Motivation

The SiteSettingsLoader class does not provide methods to load the site setting entities.
Refactor the class in general.

Proposed resolution

Add methods to load the site setting entities. Refactor the class.

Comment	File	Size	Author
#11	3176345-11.patch	12.37 KB	id.conky
#5	interdiff_3-5.txt	583 bytes	marco-s
#5	add_entities_load_methods_refactoring-3176345-5.patch	13.56 KB	marco-s
#3	add_entities_load_methods_refactoring-3176345-3.patch	13.45 KB	marco-s
#2	add_entities_load_methods_refactoring-3176345-2.patch	13.45 KB	marco-s

Issue fork site_settings-3176345

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3176345-refactor-sitesettingsloader-class changes, plain diff MR !1
Check out this branch

About issue forks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

12 October 2020 at 08:40

marco-s created an issue. See original summary.

Comment #2

marco-s

Switzerland🇨🇭

CreditAttribution: marco-s at Namics commented 12 October 2020 at 09:03

File	Size
add_entities_load_methods_refactoring-3176345-2.patch	13.45 KB

Patch #2 includes:

New methods loadAllEntities(), loadEntitiesByType(), doesEntityOfTypeExists()
New class SiteSettingsCache(). The cache logic from SiteSettingsLoader class has been extracted to this class
Some refactoring

Comment #3

marco-s

Switzerland🇨🇭

CreditAttribution: marco-s at Namics commented 12 October 2020 at 09:07

File	Size
add_entities_load_methods_refactoring-3176345-3.patch	13.45 KB

Hm, the second patch upload did not override my first added patch. Here is the right patch.

Comment #4

orlando.thoeny

Switzerland 🇨🇭

CreditAttribution: orlando.thoeny at Namics commented 12 October 2020 at 13:41

Works for me.
We should have a look at the tests and fix them.

Comment #5

marco-s

Switzerland🇨🇭

CreditAttribution: marco-s at Namics commented 13 October 2020 at 09:42

Status:

Active

» Needs review

File	Size
add_entities_load_methods_refactoring-3176345-5.patch	13.56 KB
interdiff_3-5.txt	583 bytes

Forgot a variable name replacement. Fixed in this patch.

Comment #6

scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commented 1 December 2020 at 07:38

Thanks for the work on this Marco-s!

+++ b/src/SiteSettingsLoader.php
@@ -43,19 +29,89 @@ class SiteSettingsLoader {
+  /**
+   * Checks if a site setting entity exists for the given type.
+   *
+   * @param string $type
+   *   The site setting type.
+   *
+   * @return bool
+   *   TRUE if exists, FALSE otherwise.
+   *
+   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
+   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
+   */
+  public function doesEntityOfTypeExists($type) {
+    return !empty($this->loadEntitiesByType($type));
   }

I couldn't see where this is used, is this specific to a use case you have? Not against it, just wanting to understand the goal.

For the rest, it seems to work fine, I've tested it out on a couple larger sites and haven't been able to find any issues.

Comment #7

marco-s

Switzerland🇨🇭

CreditAttribution: marco-s at Namics commented 5 January 2021 at 15:46

Hi scott_euser

Yes, we have use cases for doesEntityOfTypeExists(). For example if you reference a site settings type in code or in config. In such a case, you want to check first if the site setting entity exists (e.g. before executing further steps).

Comment #8

6 January 2021 at 08:30

marco-s opened merge request !1

Comment #9

marco-s

Switzerland🇨🇭

CreditAttribution: marco-s at Namics commented 6 January 2021 at 08:51

I cast the changes into a merge request !1

Comment #10

marco-s

Switzerland🇨🇭

CreditAttribution: marco-s at Namics commented 6 January 2021 at 09:11

We sometimes have problems with a website that has a lot of traffic. Site settings causes Call to a member function isTranslatable() on null in SqlContentEntityStorage during drush cr, because $definitions is empty (see also 2928108). We have not yet been able to find the real cause. But I try to reduce the EntityTypeManager loadMultiple() calls. With this change ( f560b762) I reduce the calls in our case from 48 calls to just 2.

Comment #11

id.conky CreditAttribution: id.conky at Drupal Ukraine Community, DevBranch for Soapbox Communications Ltd commented 13 January 2022 at 16:42

File	Size
3176345-11.patch	12.37 KB

Hi!
I also think that settings loader class should be refactored for usability and performance purposes.
You guys have a good ideas, but for my needs I decided to do it in other, simpler manner - by moving all settings to be stored in Drupal static cache.

Patch with changes attached.

Comment #12

scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commented 14 January 2022 at 15:57

Status:

Needs review

» Needs work

Thanks for your work on this id.conky and marco-s. Apologies for the silence for so long, having a child has reduced my free time a bit :)

id.conky, I think we need a combination of your approach; I understand the static cache will further improve performance within a single page load. However it removes the benefits of the cache across page loads. Site Settings rarely change in most use cases and the loader as is caches a much lighter version of the data compared to full entities (which has its downsides as well of course). I would prefer to see your use of drupal_static added in to marco-s' work on the SiteSettingsCache class.

Comment #13

scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commented 21 October 2023 at 06:42

FYI progress on new loader that works more like standard Drupal rendering happening here https://www.drupal.org/project/site_settings/issues/3395787

Does not stop refactoring this loader as long as output is maintained as is for this loader to avoid breaking all the existing sites that use this module.