Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.The \Drupal\Core\Field\PluginSettingsBase::getThirdPartySettings method returns null if the module does not exist instead of empty array.
The description of the method in the ThirdPartySettingsInterface states:
/**
* Gets all third-party settings of a given module.
*
* @param string $module
* The module providing the third-party settings.
*
* @return array
* An array of key-value pairs.
*/
public function getThirdPartySettings($module);
Changing the implementation is better here because
- By default this code return an empty array if no module is passed in and there is third party settings.
- The correlating code in ConfigEntityBase::getThirdPartySettings($module) returns an empty array if the module has no settings.
So returning an empty array is more consistent than returning a NULL. Also if there is data for the module in any of the above cases it will be an array.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2632314-21.patch | 1.9 KB | alexpott |
| #21 | 2632314-21.test-only.patch | 1.26 KB | alexpott |
| #7 | 2632314-7.patch | 655 bytes | amateescu |
| pluginbase-thirdpartysettings.patch | 467 bytes | Anonymous (not verified) |
Comments
Comment #1
Anonymous (not verified) commentedivanjaros created an issue. See original summary.
Comment #2
Anonymous (not verified) commentedComment #3
amateescu commented\Drupal\Core\Config\Entity\ConfigEntityBase::getThirdPartySettings()(correctly) returns an array as well, nice find :)Comment #5
amateescu commentedThe patch still applies fine locally on 8.0.x, must have been a bot fluke.
Comment #7
amateescu commentedI don't get it :/ Let's try a reroll..
Comment #8
swentel commentedHmm, this might break implementations in contrib/custom because it might rely on the fact that it runs NULL ? Tricky.
OTOH, this would make it consistent with getThirdPartySettings in ConfigEntityBase
Comment #9
amateescu commentedWell, the interface says that the return type is an array, so if anyone relies on the current NULL return value I'd say it is a bug in their code..
Comment #10
swentel commentedHmm yeah, I also think there won't be that many implementations yet using third party settings on fields anyway, so +1 from me.
Comment #11
catchComment #12
catchThere's potential breakage both directions:
- if someone relies on the interface return value, and we change it, we'll break that
- if someone relies on the current implementation, and we change it, we'll break that.
Both are extremely unlikely though, so I think we should just change to whichever makes most sense. Also the quicker we change it the less likely that either consuming or implementing code will be doing either.
Comment #13
swentel commentedWell given how #2625138: EntityRepository::loadEntityByUuid() implementation does not match EntityRepositoryInterface::loadEntityByUuid() @return documentation turned out, our safe bet is probably to change the docs, that should normally break 'less'.
Comment #14
catchHmm nothing broke there, just realised it wasn't as straightforward as I thought after committing.
Comment #15
swentel commentedOh wait, looking at the last patch there, it actually proposes to fix the implementation, so, I guess the patch we have here follows that pattern. However, it's silent now over there for a month and a half :)
Comment #16
eric_a commentedI guess this one is very, very similar to the fix from #2625138: EntityRepository::loadEntityByUuid() implementation does not match EntityRepositoryInterface::loadEntityByUuid() @return documentation that got into 8.1.x and 8.2.x. Moving to 8.1.x.
Comment #17
eric_a commentedSo this issue respects the contract and improves consistency by fixing the return value for an empty collection from NULL to empty array and the other issue made the implementation respect the contract and improved consistency by changing the return value for trying to load a non-existing object from FALSE to NULL. Potential breakage is extremely unlikely and the sooner fixed the better.
Patch in #7 passes tests for all branches.
Comment #18
alexpottUpdated issue summary to make a rationale for the approach. I guess there is a question about whether or not we should test this.
Comment #19
alexpottLet's take the opportunity to add a unit test of PluginSettingsBase::getThirdPartySettings()
Comment #20
xjmComment #21
alexpottHere's a test.
Comment #23
dawehnerLooks perfect for me. I hope people never rely on it to check whether a value exists or not.
Comment #26
catchCommitted/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!