silverstripe/silverstripe-cms — pull request #181 by tractorcow (2012-08-16 21:39:26+00:00)
Title: Translation Refactoring: Site Config

This request handles the removal of all hooks to the Translatable module for the SiteConfig within the core CMS module. This includes any locale-specific code that would no longer be necessary if the two modules acted independently.

Apologies for the whitespace changes showing up as diffs.

This pull request follows, and should ideally be included with, the matching pull request to the Translatable module at https://github.com/silverstripe/silverstripe-translatable/pull/44. The CMS changes can be pulled safely in advance, as only sites relying on Translatable module will be affected.

Below describes the replacement translation module functionality for the SiteConfig.

To elaborate on the mechanism I've chosen, the translation (and automatic creation) of SiteConfigs can occur during these cases:
* During dev/build
* During default database setup
* When viewing the setting tab in the CMS using ?locale=xx_XX in a locale with no siteconfig
* When browsing the site when no siteconfig has been created

In these cases SiteConfig::current_site_config will automatically create a new instance of a SiteConfig object and save it. In the past the creation of this object would have been directly delegated to the Translatable module, but in the interest of separating these tasks I have chosen to rewrite the SiteConfig code to be agnostic to Translatable's existence.

There were three requirements I held to in developing the new mechanism:
* Ensure that all SiteConfig objects belong to the same group (there is one exception noted in the code)
* Defaults should be correctly inherited (see note below)
* Translation should use the createTranslation method to remain consistent.

To facilitate the injection of Translation functionality into SiteConfig I've chosen to use the populateDefaults mechanism to handle field translation. I have also refactored the default value code for the SiteConfig object (see code in SiteConfig::populateDefaults()) to work as per convention. Potentially this code could be re-written to operate on onBeforeWrite, but there are other issues involved with this (not overwriting field values specified when editing a SiteConfig, for instance).

I have also removed any Locale fields from the SiteConfig code, as the only intent for these was to consider the requirements of the Translatable module. Instead of passing this field during creation, the current locale is used. This seems to work just as well during my tests. Test cases were updated to set the locale this way instead of directly specifying it.

In Translatable the populateSiteConfig function does most of the grunt work here; A base (usually of the default locale) translation of the SiteConfig is retrieved and has createTranslation called on it to build the new translation. Since an object by this time has already been created the code uses the update() function to copy the field values across from the new translation to the object being rendered. This also has the effect of re-using the existing ID, effectively stealing the identity of this new translation and giving it to the SiteConfig being built.

Test cases for SiteConfig (and Translatable) have been run and updated.

Note: There is a possible bug in migration of values across SiteConfigs. If a field is null in the original SiteConfig, the translation will not inherit the null, but will rather use the "default" value for that field. E.g. If the TagLine is set to null and a translation created, this field will be given "your tagline here" as a value. Can someone please confirm if this is a bug, or should this be expected behaviour? If this is a bug then I will investigate migration of nulls.

Note: There is also a bug I noted in the main code where singleton() would incorrectly populate the default values for an object. As noted in the code itself any singleton() should NOT have populateDefaults called on it (see documentation on DataObject's constructor). This unfortunately caused a crash when building a temporary database during testing, so there is a little "hack" fix in there for the mean time. Ideally the singleton() method should be fixed to behave as documented. createTranslation also triggers the populateDefaults function. This also has the disadvantage of causing a new translation to be created (if one does not already exist in the current locale) whenever singleton(SiteConfig) is called.