Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, larowlan-review.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
660 bytes
14.63 KB

One line fix...

Gábor Hojtsy’s picture

Status: Needs review » Fixed

Yay, committed!

tstoeckler’s picture

Status: Fixed » Needs review
FileSize
2.89 KB

I was in the middle of reviewing this, but here's a follow-up. :-)

  1. 5. was missing from @larowlan's review.
  2. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -11,9 +11,11 @@ use Drupal\Core\Config\ConfigFactory;
     use Symfony\Component\HttpFoundation\Request;
     
    +
     /**
    

    :-(

  3. +++ b/lib/Drupal/config_translation/ConfigMapperInterface.php
    @@ -82,7 +82,7 @@ interface ConfigMapperInterface extends ContainerFactoryPluginInterface {
    +   * Checks that all pieces of this configuration mapper have schema.
    

    have *a* schema, maybe?

  4. +++ b/lib/Drupal/config_translation/ConfigNamesMapper.php
    @@ -196,6 +207,10 @@ class ConfigNamesMapper extends DependencySerialization implements ConfigMapperI
       /**
        * {@inheritdoc}
    +   *
    +   * @throws \RuntimeException
    +   *   Throws an exception if the language codes in the config files don't
    +   *   match.
        */
    

    I think this should actually be documented on the interface. Also the exception bubbles up to getLanguageWithFallback() so should be documented there as well.

Status: Needs review » Needs work

The last submitted patch, 2114439-follow-up.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.67 KB

Mr. Hojtsy going around committing follow-up fixes without telling anyone... incredible... :-)

(the extra newline was already removed in the meantime :-))

Gábor Hojtsy’s picture

Status: Needs review » Fixed

Heh, committed this one too :)

Gábor Hojtsy’s picture

Status: Fixed » Needs review
FileSize
3.15 KB
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Committed this.

Gábor Hojtsy’s picture

#2112575: Add a DerivativeBase in the Core namespace with t() as a helper method would put the t() in DerivativeBase the way I did in #8, so we need to watch out for that...

Status: Fixed » Closed (fixed)

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