Problem/Motivation
The module currently uses \Drupal::state()
in many places but should IMO use config instead.
There are several issues by using Drupal::state() for the provider settings:
1. When a DB is copied from PROD to another environment (local, acceptance, etc), the key_value
table holding the data for \Drupal::state()
is retained.
So the whole provider config including api_mode (simulation/real), and even api_url, api_username and api_password (if not using the settings.php way) is retained. Which means one can easily end up on a PROD setup while being on another environment and break/delete/complete any translation jobs.
2. Storing this in state duplicates what is already stored in the TMGMT provider config (in tmgmt.translator.tmgmt_cdt.yml
via \Drupal\tmgmt\Form\TranslatorForm::save
).
The config stored there is actually ignored since values are actually retrieved from Drupal:state() throughout tmgmt_cdt.
3. Because of this, it is not possible to use config overrides or config split and have different settings per environment.
Right now api_url, api_username and api_password can be retrieved from settings.php (if given) but api_mode which is essential as well is not included. A quick fix could be to read it from settings.php but a better way IMO would be to use config override.
Config override would also allow for example, different proxy settings, etc...
One example of what could be a PROD config override:
$config['tmgmt.translator.tmgmt_cdt']['conf_mode']['api_mode'] = \Drupal\tmgmt_cdt\TmgmtCdtHelper::PRODUCTION;
$config['tmgmt.translator.tmgmt_cdt']['conf_credentials']['api_url'] = ...
$config['tmgmt.translator.tmgmt_cdt']['conf_credentials']['api_username'] = ...
$config['tmgmt.translator.tmgmt_cdt']['conf_credentials']['api_password'] = ...
$config['tmgmt.translator.tmgmt_cdt']['conf_advanced']['debug'] = 0;
$config['tmgmt.translator.tmgmt_cdt']['conf_network']['curl_proxy_host'] = ...
$config['tmgmt.translator.tmgmt_cdt']['conf_network']['curl_proxy_port'] = ...
$config['tmgmt.translator.tmgmt_cdt']['conf_network']['curl_proxy_user'] = ...
$config['tmgmt.translator.tmgmt_cdt']['conf_network']['curl_proxy_pass'] = ...
...
4. It prevents updating CDT settings via code: if we want to update/change the CDT settings on PROD, the only way is to go there and do the changes manually via UI. Any changes that happened in config (code) doesn't have any effect and is misleading for developers.
Normally the config:import
part during the deployment should take care of that.
Proposed resolution
Read and store the provider settings from/to config instead of state
=> replace all \Drupal::state()
calls, TMGMT already does the saving part for us.
For the credentials (api_url, api_username, api_password), probably the most secure is to enforce the use of config override to set them and validate it if api_mode = real.
For the rest (e.g: tmgmt_cdt.api_access_token, tmgmt_cdt.api_access_token_expire, tmgmt_cdt.ready,...), decide on the appropriate storage (cache seems appropriate?).