Problem/Motivation

I'm porting a module to D8, leveraging the ThirdPartySettings goodness on config entities. Un fortunately there is no way to retrieve all third party settings a module has stored. There is only a method to get a single setting, but you have to know its key.

Proposed resolution

Add a method to get all third party settings stored by a module.
In the future, modules may even want to retrieve all third party settings, regardless of the module that stored it. While we're at it, we can make that possible too.

Remaining tasks

I'm adding a first patch, but there are some details I'm not sure about, mainly because I wonder what is most consistent with the rest of Drupal core, and OOPHP at large. ToDo:
- Decide on the method name: getAllThirdPartySettings (does not start with "getThirdParty..." like the others) / getThirdPartySettings (only 1 charachter difference from "getThirdPartySetting") / getThirdPartySettingsByModule (makes less sense if we allow $module to be empty) done
- Decide if we also want to add a way to retrieve all third party settings, not filtered by module. done
- If yes, decide if that should be a separate method or simply the return value if the module name argument is empty. n/a
- Tests done

User interface changes

none

API changes

Addition of one method.

CommentFileSizeAuthor
#9 third_party_settings_get_all-2345879-8.patch2.19 KBmarcvangend
#9 interdiff-2345879-6-8.txt893 bytesmarcvangend
#6 third_party_settings_get_all-2345879-6.patch2.24 KBmarcvangend
#1 third_party_settings_get_all-2345879-1.patch1.01 KBmarcvangend
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
Assigned: marcvangend » Unassigned
Status: Active » Needs review
FileSize
third_party_settings_get_all-2345879-1.patch1.01 KB

Patch attached.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Yeah I think this was an oversight that we don't have this method. Needs unit tests for the new method like the old ones have. Also:

+++ b/core/lib/Drupal/Core/Config/Entity/ThirdPartySettingsTrait.php
@@ -68,6 +68,27 @@ public function getThirdPartySetting($module, $key, $default = NULL) {
+   * @return array
+   *   An array of key-value pairs.

Not true if the module name was empty.

+++ b/core/lib/Drupal/Core/Config/Entity/ThirdPartySettingsTrait.php
@@ -68,6 +68,27 @@ public function getThirdPartySetting($module, $key, $default = NULL) {
+   * Gets all third-party settings of the config entity, filtered by module.

Do we say on other comments that this deals with a config entity? I think it was meant to be a very generic trait.

yched’s picture

yched CreditAttribution: yched commented 
+  public function getAllThirdPartySettings($module = '') {
+    if (empty($module)) {
+      return $this->third_party_settings;
+    }

Do we really want to support that case ?

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented

Re #2:

Not true if the module name was empty.

You're right. That's a good reason to separate the get-all-settings-of-all-modules method from the get-all-settings-of-one-module method (if we want the latter at all).

Do we say on other comments that this deals with a config entity?

Good point. The inline docs for the $third_party_settings property does say "Third party entity settings", but there are no explicit references to config entities. I'll remove it.

Re #3:

Do we really want to support that case ?

I don't know, that's why that decision is one of the remaining tasks. I couldn't think of a good use case, but that doesn't prove anything. If we do not support that use case, one could still loop over the result of getThirdPartyProviders() and call getAllThirdPartySettings($module) multiple times.

yched’s picture

yched CreditAttribution: yched commented

If we do not support that use case, one could still loop over the result of getThirdPartyProviders() and call getAllThirdPartySettings($module) multiple times.

Yes. The "third-party settings" API is oriented towards "by module", so I'd vote for just getThirdPartySettings($module != NULL), and keep leave hypothetical / weird uses of "get all settings across all modules" resort to the above.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
FileSize
third_party_settings_get_all-2345879-6.patch2.24 KB

Thanks Yves. How does this look?

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
Status: Needs work » Needs review
yched’s picture

yched CreditAttribution: yched commented

Nitpick : 

+++ b/core/lib/Drupal/Core/Config/Entity/ThirdPartySettingsTrait.php
@@ -68,6 +68,24 @@ public function getThirdPartySetting($module, $key, $default = NULL) {
+   * @param string $module
+   *   The module providing the third-party setting.

Phrasing could probably be enhanced.

Other than that, looks ready to go :-)

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
Issue summary: View changes
Issue tags: -Needs tests +Amsterdam2014
FileSize
interdiff-2345879-6-8.txt893 bytes
third_party_settings_get_all-2345879-8.patch2.19 KB

Following up on suggestions by yched, this patch improves the wording of the inline docblock and uses a ternary if-statement.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Reviewed & tested by the community

Thanks !

alexpott’s picture

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

Committed 15cc5bd and pushed to 8.0.x. Thanks!

  • alexpott committed 15cc5bd on 8.0.x 
    Issue #2345879 by marcvangend: Added Enhance ThirdPartySettingsTrait...

Status: Fixed » Closed (fixed)

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

swentel’s picture

swentel CreditAttribution: swentel commented

The interface is missing the new method, needs a follow up issue.

swentel’s picture

swentel CreditAttribution: swentel commented

Follow up at #2367661: Follow up: ThirdPartySettingsTraitInterface missing getThirdPartySettings() method