Problem/Motivation

Found in #1533250: Many coding standards clean-ups in locale module:
in LocaleStringTest

   // Load all translations. For next queries we'll be loading only translated strings.    $only_translated = array('untranslated' => FALSE);

Looks like that statement accidentally got appended to the comment.

Proposed resolution

Move the $only_translated assignment to its own (non commented out) line, below the comment.

Remaining tasks

User interface changes

None.

API changes

No.

CommentFileSizeAuthor
#8 interdiff-2271485-3-8.txt1.9 KBamitgoyal
#8 drupal-move_localeStringTest_instruction_out_of_accidental_comment-2271485-8.patch1.05 KBamitgoyal
#3 drupal-move_localeStringTest_instruction_out_of_accidental_comment-2271485-03.patch1.16 KBjmmarquez
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +D8MI
jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez commented

I start with this

jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez commented
Status: Active » Needs review
FileSize
drupal-move_localeStringTest_instruction_out_of_accidental_comment-2271485-03.patch1.16 KB

Uploading patch.

steinmb’s picture

steinmb CreditAttribution: steinmb commented

That should do it :) though a little nitpicking. The inline comment goes beyond 80 chars.
WARNING | Line exceeds 80 characters; contains 88 characters
Perhaps it should be wrapped into two lines?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Wait, if $only_translated is currently not needed, it should just be removed, right?

steinmb’s picture

steinmb CreditAttribution: steinmb commented
Status: Needs review » Needs work

Your right.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

we need to look at the test and think.

if it is not used, maybe it should be used! and the test is not really testing what we are expecting it to.

amitgoyal’s picture

amitgoyal CreditAttribution: amitgoyal commented
Status: Needs work » Needs review
FileSize
drupal-move_localeStringTest_instruction_out_of_accidental_comment-2271485-8.patch1.05 KB
interdiff-2271485-3-8.txt1.9 KB

@YesCT - Please review attached patch which fixes #4 and #5.

From the code it seems that we don't really need "$only_translated = array('untranslated' => FALSE)".

eugenesia’s picture

eugenesia CreditAttribution: eugenesia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +LONDON_2014_JUNE
Related issues: +#1785086: Introduce a generic API for interface translation strings

I started looking at this issue at the LONDON_2014_JUNE Code Sprint, but could only complete it today.

After much study, I think the commented-out statement $only_translated = array('untranslated' => FALSE) is indeed extraneous and can be removed, as mentioned in #8 by @amitgoyal.

The commented-out statement sits in the function testStringSearchAPI(). I've looked at this function and referred to \Drupal\locale\StringStorageInterface, and I think this function adequately tests all locale string search functions of an object implementing StringStorageInterface.

I tried to find out why this extraneous statement might have been introduced, and can only conclude it was due to unfamiliarity with the locale string search API.

To get only translated strings from StringStorageInterface->getTranslations(), you need to pass in a condition of array('translated' => TRUE), and not array('untranslated' => FALSE).

The extraneous statement was introduced in the commit below. This commit created a new file LocaleStringTest.php which included the extraneous statement.

commit 662c42a418317de0c9e228289418f5809ed09b4e
Author: webchick <webchick@24967.no-reply.drupal.org>
Date:   Mon Oct 8 11:10:13 2012 -0700

    Issue #1785086 by Jose Reyero, Gábor Hojtsy, catch, loganfsmyth, nedjo, stella: Added Introduce a generic API for interface translation strings.

The relevant issue for the above commit is https://www.drupal.org/node/1785086#comment-6573684 .

So I'm setting this issue's status to RTBC.

eugenesia’s picture

eugenesia CreditAttribution: eugenesia commented

8: drupal-move_localeStringTest_instruction_out_of_accidental_comment-2271485-8.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 00afbcc and pushed to 8.x. Thanks!

  • alexpott committed 00afbcc on 8.x 
    Issue #2271485 by amitgoyal, Juanmamr | YesCT: Fixed Move...

Status: Fixed » Closed (fixed)

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