Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
tstoeckler’s picture

Status: Active » Needs review
FileSize
22.12 KB

Here we go.

Status: Needs review » Needs work

The last submitted patch, 2110491-1.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

Some comments on the changes.

  1. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
    @@ -7,14 +7,14 @@
    +use Drupal\config_translation\ConfigMapperInterface;
    +use Drupal\config_translation\ConfigMapperManager;
    +use Drupal\config_translation\Form\ConfigTranslationDeleteForm;
    +use Drupal\config_translation\Form\ConfigTranslationManageForm;
     use Drupal\Core\Config\Config;
     use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
     use Drupal\Core\Controller\ControllerBase;
     use Drupal\Core\Language\Language;
    -use Drupal\config_translation\ConfigNamesMapper;
    -use Drupal\config_translation\Form\ConfigTranslationDeleteForm;
    -use Drupal\config_translation\Form\ConfigTranslationManageForm;
    -use Drupal\config_translation\ConfigMapperManager;
     use Symfony\Component\DependencyInjection\ContainerInterface;
     use Symfony\Component\HttpFoundation\Request;
    

    The only real change here is ConfigNamesMapper -> ConfigMapperInterface. I reformatted the rest to be alphabetically, as that's what PhpStorm does automatically.

  2. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
    @@ -206,8 +206,7 @@ class ConfigTranslationController extends ControllerBase implements ContainerInj
    -    $locale_storage = \Drupal::service('locale.storage');
    -    $build = drupal_get_form(new ConfigTranslationManageForm($locale_storage, $this->moduleHandler()), $mapper, $target_language, $base_config);
    +    $build = drupal_get_form(ConfigTranslationManageForm::create(\Drupal::getContainer()), $mapper, $target_language, $base_config);
    
    @@ -223,8 +222,8 @@ class ConfigTranslationController extends ControllerBase implements ContainerInj
    -    return drupal_get_form(new ConfigTranslationDeleteForm(), $mapper, $target_language);
    ...
    +    return drupal_get_form(ConfigTranslationDeleteForm::create(\Drupal::getContainer()), $mapper, $target_language);
    

    The alternative to calling \Drupal::getContainer() would be injecting all the services needed by the forms into the parent controller. But once we can finally remove our hook_menu() implementation, the forms will be on separate routes anyway, and then they will need the create() anyway, so I think this is the most future-proof way to do it.

  3. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationDeleteForm.php
    @@ -7,11 +7,14 @@
    +use Drupal\config_translation\ConfigMapperInterface;
    +use Drupal\Core\Cache\Cache;
    +use Drupal\Core\Config\StorageInterface;
    +use Drupal\Core\Extension\ModuleHandlerInterface;
     use Drupal\Core\Form\ConfirmFormBase;
     use Drupal\Core\Language\Language;
    -use Drupal\config_translation\ConfigNamesMapper;
    +use Symfony\Component\DependencyInjection\ContainerInterface;
     use Symfony\Component\HttpFoundation\Request;
    -use Symfony\Component\Routing\Route;
    

    See above.

  4. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
    @@ -7,23 +7,46 @@
    +use Drupal\config_translation\ConfigMapperInterface;
     use Drupal\Core\Config\Config;
     use Drupal\Core\Config\Schema\Element;
    -use Drupal\Core\Extension\ModuleHandler;
    -use Drupal\Core\Form\FormInterface;
    +use Drupal\Core\Config\TypedConfigManager;
    +use Drupal\Core\Extension\ModuleHandlerInterface;
    +use Drupal\Core\Form\FormBase;
     use Drupal\Core\Language\Language;
    -use Drupal\config_translation\ConfigNamesMapper;
     use Drupal\locale\StringStorageInterface;
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    

    See above. Also ModuleHandler -> ModuleHandlerInterface and FormInterface -> FormBase.

  5. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
    @@ -86,55 +109,49 @@ class ConfigTranslationManageForm implements FormInterface {
    -    foreach ($this->mapper->getNames() as $id => $name) {
    -      $form[$id] = array(
    +    foreach ($this->mapper->getNames() as $name) {
    +      $form['config_names'][$name] = array(
    ...
    -      $form[$id] += $this->buildConfigForm(config_typed()->get($name), config($name)->get(), $this->baseConfigData[$name]);
    +      $form['config_names'][$name] += $this->buildConfigForm($this->typedConfigManager->get($name), $this->config($name)->get(), $this->baseConfigData[$name]);
    

    There are two changes here:
    1. Putting all of the configurations into a separate form key. I think this is best practice, since we're adding other top-level form keys below.

    2. Instead of keying by $id, which is just a random array key, I think it makes sense to key by something meaningful. That will make altering so much easier.

  6. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
    @@ -86,55 +109,49 @@ class ConfigTranslationManageForm implements FormInterface {
    -    $form_values = $form_state['values'];
    +    $form_values = $form_state['values']['config_name'];
    

    Looking at this now, this will not work, because I did not set #tree on $form['config_names'];

  7. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
    @@ -86,55 +109,49 @@ class ConfigTranslationManageForm implements FormInterface {
    -    foreach ($this->mapper->getNames() as $id => $name) {
    +    foreach ($this->mapper->getNames() as $name) {
    ...
    -      $this->setConfig($this->language, $base_config, $translation_config, $form_values[$id], !empty($locations));
    +      $this->setConfig($this->language, $base_config, $translation_config, $form_values[$name], !empty($locations));
    

    See above.

  8. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
    @@ -148,7 +165,11 @@ class ConfigTranslationManageForm implements FormInterface {
    -    drupal_set_message(t('Updated @language configuration translations successfully.', array('@language' => $this->language->name)));
    +    // @todo A different message should be displayed in the following cases:
    +    //   1. A translation was added.
    +    //   2. A translation was deleted, due to missing overrides.
    +    //   3. A translation was not created, due to missing overrides.
    +    drupal_set_message($this->t('Updated @language configuration translations successfully.', array('@language' => $this->language->name)));
    

    I realized this when testing the another issue. Thinking about this some more, I think it would make even more sense to use a validate() method to prevent cases 2. and 3.

  9. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
    @@ -232,16 +253,14 @@ class ConfigTranslationManageForm implements FormInterface {
    -        $form_element_class = (isset($definition['form_element_class']) && class_exists($definition['form_element_class'])) ? $definition['form_element_class'] : '\Drupal\config_translation\FormElement\Textfield';
    +        $definition += array('form_element_class' => '\Drupal\config_translation\FormElement\Textfield');
    

    I think this makes the code much easier to read. It removes the class_exists() but that's just babysitting broken code IMO.

  10. +++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
    @@ -232,16 +253,14 @@ class ConfigTranslationManageForm implements FormInterface {
    -        /**
    -         * @var \Drupal\config_translation\FormElement\Element $form_element;
    -         */
    -        $form_element = new $form_element_class();
    +        /** @var \Drupal\config_translation\FormElement\Element $form_element */
    +        $form_element = new $definition['form_element_class']();
    

    This is sort of my personal preference, but I think it's very uncommon to have full docblocks in inline code, so I think this will find more agreement in general.

tstoeckler’s picture

This fixes some tests and #4.6

Status: Needs review » Needs work

The last submitted patch, 2110491-1-5-interdiff.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
6.64 KB
41.9 KB

Running tests takes a while on my machine, so let's just try this one in the meantime.

Status: Needs review » Needs work

The last submitted patch, 2110501-3.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
6.26 KB
30.02 KB

Oops, wrong patch.

Status: Needs review » Needs work

The last submitted patch, 2110491-7.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
733 bytes
30.02 KB

Well, that was dumb. Still haven't run this locally :-/

Status: Needs review » Needs work

The last submitted patch, 2110491-11.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
30.58 KB

All right, fixed the remaining fails. One was due to #1958754: Copy improvements to default Views so basically all patches should currently fail for that. See the $description change in the interdiff.

tstoeckler’s picture

Yay, RTBC anyone? :-)

tstoeckler’s picture

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.php
@@ -49,31 +72,31 @@ class ConfigTranslationManageForm implements FormInterface {
    * Creates manage form object with string translation storage.
    *
    * @param StringStorageInterface $locale_storage
...
+  public function __construct(TypedConfigManager $typed_config_manager, StringStorageInterface $locale_storage, ModuleHandlerInterface $module_handler) {

Ahh, the $typed_config_manager is missing from the docs.

Quick re-roll for that.

Gábor Hojtsy’s picture

Status: Needs review » Fixed
Issue tags: +D8MI, +language-config
FileSize
2.39 KB

Yay, thanks! I committed this with the minor changes attached.

1. Added missing $this->t conversions.

2. Removed a @todo about the different messages you wanted to add. I don't agree that we'd need different messages. If the translations happen to be exactly the same as the sources, I think it makes sense for us to remove the file for performance at least. We could also keep an empty file if we are concerned for a tool that would discover "untranslated" things would get the wrong impression. But how we store when translations are identical to sources is not really the business of the translator IMHO. I don't think we should inform them that now we *deleted* translations, because what happened is the translation is the same as the source.

3. Edited an existing TODO, since that issue seems to not be getting to core with some seemingly good reasons :)

It would be great to continue with other modernizations, eg. #2104925: Convert to work with routes instead of paths in mappers or more test coverage at #2095383: Improve config translation test coverage by adding a test module :)

Gábor Hojtsy’s picture

Or figuring out the AJAX callback and test coverage at #2099997: Use config translation to translate date formats which are missing for that to move :)

++ :)

tstoeckler’s picture

2. Removed a @todo about the different messages you wanted to add. I don't agree that we'd need different messages. If the translations happen to be exactly the same as the sources, I think it makes sense for us to remove the file for performance at least. We could also keep an empty file if we are concerned for a tool that would discover "untranslated" things would get the wrong impression. But how we store when translations are identical to sources is not really the business of the translator IMHO. I don't think we should inform them that now we *deleted* translations, because what happened is the translation is the same as the source.

Well at the very least we should display a different message when a translation is *added* (vs. updated), anything else is just weird.

I still think we should do some validation on an "empty" translation, but I'm fine with being overruled on that detail.

In general I'm still working on the automatic config form discovery, I just came across some of this there so I spun it off.

Thanks for the quick commit.

Gábor Hojtsy’s picture

Yeah adding vs. updating sounds very logical to have separate messages yup :)

Gábor Hojtsy’s picture

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