Follow up for #2017985: ConfigTranslationManageForm.php standards cleanup to get ready for getting into core

Updated: Comment #0

Problem/Motivation

We sometimes use the short word config instead of the english word configuration in our docs.

Proposed resolution

config -> configuration in all our docs

Remaining tasks

See if there are places we might want to keep config. Suspect, those would only be for actual class names, Config.

User interface changes

No.

API changes

No.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LinL’s picture

Status: Active » Needs review
FileSize
13.66 KB

Here's a patch for this. The only one I (intentionally!) left is in this todo:

   // @todo Do we need to flush caches with drupal_flush_all_caches()? The
    //   config change may affect page display.

in ConfigTranslationDeleteForm.php

YesCT’s picture

Assigned: YesCT » Unassigned
Xano’s picture

@LinL Apparently we started working on this simultaneously :)

My patch seems to do exactly what #1 does, but it also replaces some internal variable names. It does not touch any API-related code.

LinL’s picture

@Xano :) Good that they agree!

I noticed this one extra change I had in #1:

--- a/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php
+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php
@@ -332,7 +332,7 @@ class ConfigTranslationListUITest extends WebTestBase {
   }
   /*
-   * Tests all lists of config to see if translate link is added to operations.
+   * Tests configuration lists to see if translate link is added to operations.
    */
YesCT’s picture

Issue tags: -Novice

Why should we even invest in this? Is it worth it?

In part,
I might "config" something. that's configure.
I might check the "config". that's configuration.

If someone tries to put comments into a translation tool, like google translate, to understand better what the comments mean, will it translate "config" right?
I tried some comment blocks, and translating to spanish, it seems to understand that config is configuration.

=====

So a Config object is really just: Config object, Config is the name of the class.
But it's a configuration object, describing what that is in words.

------

+++ b/lib/Drupal/config_translation/Access/ConfigNameCheck.phpundefined
@@ -12,7 +12,7 @@ use Symfony\Component\Routing\Route;
- * Checks access for displaying config translation page.
+ * Checks access for displaying configuration translation page.

Should we add articles like "a" and "the" while we are changing these lines?

Checks access for displaying the configuration translation page.

------------

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationDeleteForm.phpundefined
@@ -78,7 +78,7 @@ class ConfigTranslationDeleteForm extends ConfirmFormBase {
-    //   config change may affect page display.
+    // configuration change may affect page display.

actually, @todo's have additional lines indented 2 spaces. https://drupal.org/node/1354#todo

-------------

+++ b/lib/Drupal/config_translation/ConfigGroupMapper.phpundefined
+++ b/lib/Drupal/config_translation/ConfigGroupMapper.phpundefined
@@ -126,7 +126,7 @@ class ConfigGroupMapper implements ConfigMapperInterface {

@@ -126,7 +126,7 @@ class ConfigGroupMapper implements ConfigMapperInterface {
    * {@inheritdoc}
    */
   public function getConfigGroup($arg = NULL) {
-    // This is already a config group.
+    // This is already a configuration group.

tricky thing words and names of things.

Take this one.

Let's keep it ConfigGroupMapper and not...
ConfigurationGroupMapper implements ConfigurationMapperInterface
public function getConfigurationGroup

And changing it from
This is already a config group.
to
This is already a configuration group.
is a good change.

The alternative would be instead:
This is already a ConfigGroupMapper.

--- aside ---
Also, why is it a "group"?

 * Configuration editing screen mapper.
 */
class ConfigGroupMapper implements ConfigMapperInterface {

Group is not used to describe this ConfigGroupMapper. Maybe that should be Mapper for the editing screen of a group of configuration.
--- end aside ---

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -174,7 +174,7 @@ class ConfigTranslationController implements ControllerInterface {
-    $base_config = $group->getConfigData();
+    $base_configuration = $group->getConfigData();

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -77,17 +77,17 @@ class ConfigTranslationManageForm implements FormInterface {
-    $this->baseConfigData = $base_config_data;
+    $this->baseConfigData = $base_configuration_data;

Here a couple examples of code using config vs configuration.
In this case, getConfigData is returning an array of configuration information, and not a specific Config class, so I think $base_configuration makes sense.

------

I didn't look at every change in the patch.

YesCT’s picture

Title: docs cleanup use configuration instead of config, to get ready for getting into core » docs cleanup use configuration instead of config

core is already inconsistant in how it does this.

Xano’s picture

YesCT’s picture

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.phpundefined
@@ -332,7 +332,8 @@ class ConfigTranslationListUITest extends WebTestBase {
-   * Tests all lists of config to see if translate link is added to operations.
+   * Tests all lists of configuration to see if translate link is added to
+   * operations.

Yeah, the trouble with long words... makes one line summaries longer than one line.

We should shorten this line. For the one line summary, if needed can add a blank line and then a paragraph for more explanation to make sure we retain all the info.

I just reworded it.

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -261,22 +261,22 @@ class ConfigTranslationManageForm implements FormInterface {
-        // If the config file being translated was originally shipped, we should
+        // If the configuration file being translated was originally shipped, we should
         // update the locale translation storage. The string should already be

this needs to wrap at 80 chars too.

---------
I read the whole patch.
I rechecked all the files for short config in the comments and did not see any.
I dont feel strongly about the $configuration variable names, so I'm fine with it like this.
We dont want to drive our selves crazy, so I think we are ok this.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I think renaming the variables is a mistake. The core function we get configuration from is config(). The namespace is Drupal\Core\Config\Config, the module name is config_translation, and so on and on. I think config is well understood in this sense. I see that the comments are cleaner to spell out the full word, but the API does not do that, so I'd not rename variables on this either. That would also make this patch *much* easier to review.

YesCT’s picture

Assigned: Unassigned » YesCT

ok. I'll do it.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
21.77 KB

here is just a reroll.

Status: Needs review » Needs work

The last submitted patch, config_translation_2038743_11.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
15.2 KB

not doing the variable renames.

unrelated failure should be fixed by #2054183: shortcut renamed

YesCT’s picture

oops. missed a set.

Status: Needs review » Needs work

The last submitted patch, config_translation_2038743_14.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Yay, thanks, looks good to me, committed!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.