Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Status: Active » Needs review
Issue tags: +Novice, +Documentation, +Coding standards
FileSize
4.58 KB
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Nice cleanup!

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

just noticed.

+++ b/core/lib/Drupal/Core/Language/LanguageDefault.php
@@ -1,5 +1,10 @@
+ * Contains \Drupal\Core\Language\LanguageDefault

Needs a period at the end.

Worth having a second look if there is anything else.

znerol’s picture

Status: Needs work » Needs review
FileSize
4.58 KB

Right, rerolled.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the quick fix:) Back to RTBC.

webchick’s picture

Priority: Normal » Minor

Triaging the queue a bit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2212411-code-style-cleanup-of-language-system-3.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review
znerol’s picture

Status: Needs review » Reviewed & tested by the community

Last test-failure was a test-bot-failure. Set this to RTBC again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2212411-code-style-cleanup-of-language-system-3.patch, failed testing.

znerol’s picture

Issue tags: +Needs reroll

Tagging it for a reroll.

LinL’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.63 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 12: 2212411-code-style-cleanup-of-language-system-12.patch, failed testing.

visabhishek’s picture

Status: Needs work » Needs review
plach’s picture

Just a few remarks, otherwise looks good, thanks.

  1. +++ b/core/lib/Drupal/Core/Language/LanguageDefault.php
    @@ -24,10 +29,11 @@ class LanguageDefault {
    +   * @param array $options
    ...
    +  public function __construct(array $options) {
    

    In the entity system we call the initial values param $values. I'd do the same for consistency.

  2. +++ b/core/lib/Drupal/Core/Language/LanguageManagerInterface.php
    @@ -17,10 +17,10 @@
    -   * @param \use Drupal\Core\StringTranslation\TranslationInterface $translation
    +   * @param \Drupal\Core\StringTranslation\TranslationInterface $translation
    

    lol

  3. +++ b/core/lib/Drupal/Core/Language/LanguageManagerInterface.php
    @@ -77,17 +77,17 @@ public function reset($type = NULL);
    +   *   (optional) Specifies the state of the languages that have to be
    +   *   returned. It can be: Language::STATE_CONFIGURABLE,
    

    Am I wrong or now "returned." fits in the upper line?

Claudis’s picture

Status: Needs review » Reviewed & tested by the community

Patch from #12 applied. Tests cleanly.

LinL’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.63 KB
1.29 KB

Thanks @plach. Here's a reroll.

LinL’s picture

Found another comment that was running over 80 chars and a couple of typos, so here it is again.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oh cool. This isn't nearly as big of a patch as I was fearing. :) I think it's cool to let this go in despite not being in a "disruptive patch period" since most of these fixes are pretty small and self-contained.

Committed and pushed to 8.x. Thanks!

  • Commit d479a96 on 8.x by webchick:
    Issue #2212411 by LinL, znerol: Code style cleanup of language system.
    

Status: Fixed » Closed (fixed)

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