ConfigTranslationManageForm.php standards cleanup to get ready for getting into core

Form-generating functions
in https://drupal.org/node/1354#functions
use a special one line summary format different than the usual method third person verb..
these are methods that build forms. I'm not sure how those should be.

buildConfigForm needs types on these in the doc block and type hinting on the method

   * @param $schema
   *   Schema definition of configuration.
   * @param $config
   *   Configuration object of requested language.
   * @param $base_config
   *   Configuration object of base language.
   * @param bool $collapsed
   *   Flag to set collapsed state.
   * @param string|NULL $base_key
   *   Base configuration key.

Also, I think the base_key string is not NULL but '' which is the empty string and just a string still.
schema might be a string, but I think the config object is a .. class.

Oh, maybe it's \Drupal\Core\Config\Config

reordered the use's
added some "the"s to help the comments.
fixed format of {@inheritdoc} (no period on those lines and needs the curly brackets

Next: still need to actually read through this one. Stopped part way done.

is base_config the config in the source language?
it's sometimes typed as array and sometimes \Drupal\Core\Config\Config

I wonder if we should just call it source_config, or does base imply something a bit more?

----

      $element_key = implode('.', array_filter(array($base_key, $key)));

array_filter takes out either the base key, or the key, if they are null/empty? and then the implode concatenates them to make the key or just that part of the config?
I think this line could use a comment, but I dont know what it should be.

Maybe this would make more sense if $base_key was described better?

   * @param string|NULL $base_key
   *   Base configuration key.

will the name in base_config ever be NULL or '' or '0'?
If a name could be '0' it will get filtered out, and that might lead to duplicates.
I'm hoping no one names their keys '0'

I'll make a guess at a comment for it:

      // Make the specific element key, "$base_key.$key".

I'm probably over thinking this.

----

Next read through setConfig.

https://drupal.org/node/2016569, change notice for locale_storage().

#2020343: remove instance of locale_storage() is doing that.

Took out a commented dpm
Put full namespaced paths in the @param
Tried to clarify some comments.

Next: review by someone.

Patch does not apply anymore.

patching file lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
Hunk #2 FAILED at 7.
Hunk #3 succeeded at 64 with fuzz 1 (offset 17 lines).
Hunk #4 succeeded at 104 (offset 17 lines).
Hunk #5 succeeded at 119 with fuzz 1 (offset 17 lines).
Hunk #6 succeeded at 145 (offset 17 lines).
Hunk #7 succeeded at 161 (offset 17 lines).
Hunk #8 succeeded at 225 (offset 17 lines).
Hunk #9 succeeded at 246 (offset 17 lines).
Hunk #10 succeeded at 269 (offset 17 lines).
Hunk #11 succeeded at 299 (offset 17 lines).
Hunk #12 succeeded at 310 (offset 17 lines).
1 out of 12 hunks FAILED -- saving rejects to file lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php.rej

Unrelated to the scope of this patch but important:

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -132,13 +144,13 @@ class ConfigTranslationManageForm implements FormInterface {
-   * @todo
-   *   Make this conditional on the translatable schema property from
+   * @todo Make this conditional on the translatable schema property from
    *   http://drupal.org/node/1905152 - currently hardcodes label and text.

This todo should now be possible in the module, no core changes required. Should be a different issue :)

Wrong patch? Seems like one committed earlier.

the exceptions are probably from the typecasting.
Maybe types are being used, different then the documentation.

[edit]
yeah, for example:

Argument 2 passed to Drupal\config_translation\Form\ConfigTranslationManageForm::buildConfigForm() must be an instance of Drupal\Core\Config\Config, array given, called in /var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php on line 97 and defined	Recoverable fatal error	ConfigTranslationManageForm.php	167	Drupal\config_translation\Form\ConfigTranslationManageForm->buildConfigForm()	
Argument 3 passed to Drupal\config_translation\Form\ConfigTranslationManageForm::buildConfigForm() must be an instance of Drupal\Core\Config\Config, array given, called in /var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php on line 97 and defined

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -149,9 +149,9 @@
-   * @param \Drupal\Core\Config\Config $base_config
+   * @param array $base_config_data

I thought one could stay $base_config and be the Config type.

I think this looks all good changes. Only one thing:

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -149,19 +161,19 @@ class ConfigTranslationManageForm implements FormInterface {
-  protected function buildConfigForm($schema, $config, $base_config, $collapsed = FALSE, $base_key = '') {
+  protected function buildConfigForm($schema, $config_data, $base_config_data, $collapsed = FALSE, $base_key = '') {

We can/should typehint the array in the signature too?

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -254,7 +269,6 @@ class ConfigTranslationManageForm implements FormInterface {
   protected function setConfig(Language $language, Config $base_config, Config $translation_config, array $config_values, $shipped_config = FALSE) {

ah, I see this one was not in the patch. ok.

#18: config_translation.code_manageform.2017985-18.patch queued for re-testing.

this conflicted with #2028127: clean up property names to be camel case instead of underscore separated words to cleanup to get ready for getting into core

just a reroll (followed https://drupal.org/patch/reroll instructions)

addresses the second part of #7

Next steps: still need to get the type hints right.

Fails are:

Argument 2 passed to Drupal\config_translation\Form\ConfigTranslationManageForm::buildConfigForm() must be an array, string given, called in /var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php on line 176 and defined	Recoverable fatal error	ConfigTranslationManageForm.php	167	Drupal\config_translation\Form\ConfigTranslationManageForm->buildConfigForm()	
Argument 3 passed to Drupal\config_translation\Form\ConfigTranslationManageForm::buildConfigForm() must be an array, string given, called in /var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php on line 176 and defined

  protected function buildConfigForm($schema, array $config_data, array $base_config_data, $collapsed = FALSE, $base_key = '') {
    $build = array();
    foreach ($schema as $key => $element) {
      // Make the specific element key, "$base_key.$key".
      $element_key = implode('.', array_filter(array($base_key, $key)));
      $definition = $element->getDefinition() + array('label' => t('N/A'));
      if ($element instanceof Element) {
        // Build sub-structure and include it with a wrapper in the form
        // if there are any translatable elements there.
        $sub_build = $this->buildConfigForm($element, $config_data[$key], $base_config_data[$key], TRUE, $element_key);
        if (!empty($sub_build)) {

So, looks like this is recursively called, and finally, the config_data and base_config_data are strings.

I'm not totally sure I understand, but... here goes.

oh. :) silly me. I confused docs with uh.. code.

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -6,34 +7,34 @@
- * Provides a form for manage configuration translation.

correct me, if I'm wrong. 'Provides a manage configuration translation form' doesn't sound right. May be 'Provides a form to manage configuration translation' or 'Provides a configuration translation manage form'?

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -6,34 +7,34 @@
-   * The group of config translation.
+   * The group of the config translation.
    *
    * @var \Drupal\config_translation\ConfigMapperInterface
    */
   protected $group;
 
   /**
-   * The language of config translation.
+   * The language of the config translation.
    *
    * @var \Drupal\Core\Language\Language
    */
   protected $language;
 
   /**
-   * The language of config translation source.
+   * The language of the config translation source.
    *
    * @var \Drupal\Core\Language\Language
    */

config => configuration

fixing this now.

Not sure about changing config to configuration everywhere...
variables and classes.. and the module use "config".

let's think about changing config -> configuration in all our docs and do that in a separate issue.
I asked on irc and Xano thought it would be a good idea.

Committed f8e2c48 and pushed to 8.x-1.x. Thanks!

re #32
#2038743: docs cleanup use configuration instead of config