Problem/Motivation
The flattening approach works fine for simple label fields like credits, etc, but it has caused problems breaking from standard Drupal architecture as this module gets more usage.
Proposed resolution
Would suggest instead we have a loader that renders site settings entities in a standard way, respecting manage display.
It then cannot inject the site settings to every template, so keeping with the goal of the module to add labels very simply to eg footer, header, other regions where site settings are useful, suggest we add a Twig Filter and make that the recommended way. Similar to what Twig Tweak offers but without the dependency on that module perhaps, or it could be a dependency of that loader?
Remaining tasks
Provide annotation for Loader; id + label + whether to autoload or notProvide new loader with autoload disabledProvide update hook to maintain status quo for loaderLet site settings config form choose loaderOnly auto-load site settings into templates if autoload is enabled on the loaderRemove calls to site_settings.loader service, replace with plugin manager load definitionMove SiteSettingsLoaderBase functionality into FlattenedSiteSettingsLoaderMove SiteSettingsRenderer into FlattenedSiteSettingsLoaderChange /admin/content/site-settings to a ViewUpdate hook to install the View maintaining roughly the status quo for the listingAdd 'Create setting' to View header for not yet created site settingsAdd 'Add another' to operations for site settings that allow multipleBuild FullSiteSettingsLoaderAdd testsUpdate documentation
Follow-up issues
- https://www.drupal.org/project/site_settings/issues/3398584
- https://www.drupal.org/project/site_settings/issues/3399190
User interface changes
Site settings config form allows choice of loader
Data model changes
Multiple site settings loaders will be available + developers can optionally override/provide their own.
Issue fork site_settings-3395787
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
scott_euser commentedWould welcome feedback on the proposed steps.
Comment #3
scott_euser commentedComment #4
scott_euser commentedComment #5
scott_euser commentedComment #6
scott_euser commentedAm thinking this probably needs a
8.x-2.xbranch in case anyone is extending/overriding services from this module, its probably unavoidable to cause breaks there. For those not extending/overriding services, should not be a problem to have a smooth upgrade path maintaining the status quo.Comment #7
scott_euser commentedProgress is happening in the MR, crossing things off as I get through them.
Comment #9
scott_euser commentedComment #11
scott_euser commentedAdding credit to bobi-mel for type hints from https://www.drupal.org/project/site_settings/issues/3176342 - thanks!
Comment #12
scott_euser commentedComment #13
scott_euser commentedComment #14
scott_euser commentedComment #15
scott_euser commentedComment #16
scott_euser commentedComment #17
scott_euser commentedNearly there with this. Need to resolve existing test failures, add new tests to cover new functionality, and update documentation. Then follow-up with this probably https://www.drupal.org/project/site_settings/issues/3398584
Would appreciate feedback on approach and suggestions before we eventually lock in to this direction of travel.
Comment #18
scott_euser commentedComment #19
scott_euser commentedComment #20
scott_euser commentedComment #21
scott_euser commentedOkay tests are all passing now, including new tests for twig extension for the full loader.
One warning from phpstan which is ignored in core; support request raised here for help ignoring that in contrib as well: https://www.drupal.org/project/drupal/issues/3399282
Just the documentation updates to be made otherwise this is ready now for 2.0.x branch.
Comment #22
scott_euser commented