Hi,
First of all thanks for a great module, it works like a charm.
I do have a small feature request, it would be nice if the twig function 'site_setting_field(..)' was aware of the current language by default if no language code was provided.
Problem/Motivation
Some twig functions aren't aware of the current language right now.
Currently I have to inject the langcode to use it as follows:
{{ site_setting_field('[id]', '[field]', '[view_mode]', langcode) }}
Instead of ideally just:
{{ site_setting_field('[id]', '[field]') }}
That's because Drupal\site_settings\Twig\TwigExtension::singleSiteSettingsByNameAndField does the following:
if ($langcode && $site_setting->hasTranslation($langcode)) {
$site_setting = $site_setting->getTranslation($langcode);
}Proposed resolution
Let's make it so that if no langcode was provided, the current language id is given.
I suppose the above code snippet would have to become:
$langcode = $langcode ?? $this->languageManager->getCurrentLanguage()->getId();
if ($site_setting->hasTranslation($langcode)) {
$site_setting = $site_setting->getTranslation($langcode);
}(The language manager will need to be injected in the class still)
Issue fork site_settings-3507349
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
randalv commentedA little correction from my end, all the functions that call the "renderSiteSettings"-method in the extension class do seem to render with the correct language context, but the other two functions (`site_setting_field` && `site_setting_entity_by_name`) specifically check for a langcode and load that translation, so those should be altered I think.
I'll add a MR for it.
Comment #4
randalv commentedMerge request added!
I also took the liberty to remove what I assume was an orphaned `var_dump` 😇
Comment #5
randalv commentedComment #6
scott_euser commentedHa good spot on the var_dump, thank you!
Let me know if you're able to add automated test coverage to this? I believe the sample data module would need some translations added in order to make it testable. If you aren't able I'll add to my list to eventually do.
Thanks!
Comment #8
svendecabooterI have added test coverage for this.
Not sure if this belongs in LoaderLanguageTest specifically, but it had all the setup logic already available, so seemed like the easiest place to add this test.
Comment #9
scott_euser commentedCheers thank you! Added a post update hook to clear the cache for the service args change just in case per https://www.drupal.org/project/drupal/issues/2743313
Merging, thanks!
Comment #11
scott_euser commentedComment #13
svendecabooterCould a new release be tagged with this improvement?
Comment #14
scott_euser commentedSure done