Files: 
CommentFileSizeAuthor
#18 drupal-locale_coder_reviewer-1533250-17.patch119.19 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,711 pass(es), 54 fail(s), and 52 exception(s).
[ View ]
#16 2014411_locale.phpcs_.coder_review.txt16.01 KBfran seva
#13 locale.phpcs_.coder_review.txt57.18 KBfran seva
#8 drupal-make_locale_module_pass_coder_review-1533250-08.patch145.89 KBsriharsha.uppuluri
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,824 pass(es), 368 fail(s), and 122 exception(s).
[ View ]

Comments

Status:Active» Postponed

Status:Postponed» Active

The locale module is moving much closer to being in good shape from the docs API perspective. Can someone please post here the output from a Coder review of the locale module?

I am unable to run Coder at the moment and would appreciate seeing what gets sniffed as I roll a patch for some code style issues I saw while working on the Locale docs review. Thanks in advance.

I am also removing Novice tag since Coder output still requires some more advance interpretation for possible positive code sniffs.

In test LocaleCompareTest.php, there is a member call called $translation_directory. This should be converted to a camel case variable like $translationDirectory.

In LocaleExportTest.php, there is a member called $admin_user. This too should be changed to a camel case variable. Same case with LocaleImportFunctionalTest.php.

Status:Active» Postponed

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

Issue summary:View changes
Status:Postponed» Active

Assigned:Unassigned» sriharsha.uppuluri

I'm starting on coder review

Started working on the coder review

Status:Active» Needs review
StatusFileSize
new145.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,824 pass(es), 368 fail(s), and 122 exception(s).
[ View ]

Uploading patch.

Status:Needs review» Needs work

The last submitted patch, 8: drupal-make_locale_module_pass_coder_review-1533250-08.patch, failed testing.

Assigned:sriharsha.uppuluri» Unassigned

Assigned:Unassigned» fran seva

I'm going to check whats happening with the patch and the code review.
I assigned myself the issue

StatusFileSize
new746.64 KB

The phpcs founds 49 ERROR(S) AND 4 WARNING(S) AFFECTING 36 LINE(S)
Attach the file with the output.

StatusFileSize
new57.18 KB

I've attached the file with the phpcs output apply to locale module.
The file in #12 is wrong just have one file review.

The command run to get the coder reviewer was:
~/.composer/vendor/bin/phpcs --extensions=php,module,inc,install,test,profile,theme --standard=/Users/fran/.composer/vendor/drupal/coder/coder_sniffer/Drupal drupal/core/modules/locale/

@TravisCarden I see that the output check as an error the proposal change in the comment #3.
In comment #3 it's said to change non camel variable to camel but the output said the oposite. Am I using the wrong phpcs?

[:edit]

I've tried the codeSniffer standard from git code [1] and the result is the same
 Name      ExportForm.php   Location      file .../core/modules/locale/lib/Drupal/locale/Form/ExportForm.php - [drupal]   Problem synopsis      phpcs: Variable "languageName" is camel caps format. do not use mixed case (camelCase), use lower case and _ (at line 140)

[1] https://github.com/TravisCarden/coder/tree/issue-1518116

Hey, @fran seva. I can see how this could be confusing. We use camelCase on object properties but underscores in local variables, so there are times for each pattern. Sniffer is pretty good about telling you when to use which. Thanks!

@TravisCarden for the confirmation!! I'm going to check the las patch submitted comparing its changes with the sniffer output.

StatusFileSize
new16.01 KB

Hi all!!
I've been working in the review and:

* I've tried to apply the attached patch to start working with the patch applied but has been impossible...I tried to apply a reroll but after read the dedicated page to explain reroll understood I have to start from a valid patch and there is no one.

* The currently state is attached as a file. @TravisCarden I have an strange case related with LocateLookupTest.php. The Errors are really strange. If you apply the changes the result is a rare indented code. Could you confirm that it's correct?

As you can see in the attached file, the mayor errors are related to indentation and comment block (needs more coments)

FILE: ...-dev/core/modules/locale/tests/Drupal/locale/Tests/LocaleLookupTest.php
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
177 | ERROR | Line indented incorrectly; expected 35 spaces, found 27
178 | ERROR | Line indented incorrectly; expected 37 spaces, found 10
179 | ERROR | Line indented incorrectly; expected 37 spaces, found 19
181 | ERROR | Line indented incorrectly; expected 37 spaces, found 10
182 | ERROR | Line indented incorrectly; expected 37 spaces, found 19
184 | ERROR | Line indented incorrectly; expected 37 spaces, found 10
185 | ERROR | Line indented incorrectly; expected 37 spaces, found 19
187 | ERROR | Closing brace indented incorrectly; expected 27 spaces, found 8
188 | ERROR | Closing brace indented incorrectly; expected 4 spaces, found 6
--------------------------------------------------------------------------------

@TravisCarden forget what I said about the indented code I solved it.
Now just remain 29 errors, all of them "Missing function doc comment"

StatusFileSize
new119.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,711 pass(es), 54 fail(s), and 52 exception(s).
[ View ]

patch after apply all changes to pass phpcs validations.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 18: drupal-locale_coder_reviewer-1533250-17.patch, failed testing.

The test that are failing are:
* LocalUpdateTest.php
* LocalUpdateInterfaceTest.php
* LocalUpdateCronTest.php

I have run them in my local environment and there are php errors...I'm going to check them.

I'm still working on it

After review the test for LocalUpdateCronTest I think the errors could be caused by a bad configuration in locale module in my environment.
I'm going to check all the errors but I have some questions:

* Are there any section in Drupal.org with information about how configure the locale module to run all test correctly?
* The errors in the test machine for patch #18 for LocalUpdateCronTest are the same, so could be possible that the locale test for cron had an error in the initialization? I'm confuse, because if I don't apply the patch #18 the tests don't show any error

I'm thinking on it but if someone know anything about this questions will be great!!

Got as far as LocaleStringTest:

  1. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.php
    @@ -189,7 +189,8 @@ public function submitForm(array &$form, array &$form_state) {
    -      // compare the new strings with the existing strings a string in the same format is created.
    +      // compare the new strings with the existing strings a string in the
    +      // same format is created.

    This should be broken after "same", not after "the".

  2. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -121,8 +121,8 @@ protected function translateFilterLoadStrings() {
    -   * @return array $filter_values
    -   *   The filter values.
    +   * @return
    +   *   array $filter_values. The filter values.

    This should be reverted. The current code is fine as is.

  3. +++ b/core/modules/locale/lib/Drupal/locale/Locale.php
    @@ -21,6 +21,7 @@ class Locale {
        * @return \Drupal\locale\LocaleConfigManager
    +   *   The specified service

    This should be something like "The typed config manager." (don't forget the trailing period.)

  4. +++ b/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.php
    @@ -343,12 +343,14 @@ protected function dbStringLoad(array $conditions, array $options, $class) {
        *   joining the 'locales_target' table.
    +   *
        * @param array $options

    This should be removed. It can't be seen from the patch context, but both of these are @param.

  5. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.php
    @@ -23,6 +23,18 @@ class LocaleConfigTranslationTest extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    @@ -31,6 +43,9 @@ public static function getInfo() {
    +  /**
    +   * Specific sets up a for running functional and integration tests.
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php
    @@ -21,6 +21,18 @@ class LocaleContentTest extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.php
    @@ -21,6 +21,18 @@ class LocaleExportTest extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    @@ -32,13 +44,16 @@ public static function getInfo() {
    +  /**
    +   * Specific sets up a for running functional and integration tests.
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php
    @@ -21,6 +21,18 @@ class LocaleImportFunctionalTest extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    @@ -32,17 +44,20 @@ public static function getInfo() {
    +  /**
    +   * Specific sets up a for running functional and integration tests.
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php
    @@ -22,6 +22,18 @@ class LocaleJavascriptTranslation extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleLibraryInfoAlterTest.php
    @@ -22,6 +22,18 @@ class LocaleLibraryInfoAlterTest extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php
    @@ -22,6 +22,18 @@ class LocalePathTest extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    @@ -30,7 +42,10 @@ public static function getInfo() {
    +  /**
    +   * Specific sets up a for running functional and integration tests.
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePluralFormatTest.php
    @@ -21,6 +21,18 @@ class LocalePluralFormatTest extends WebTestBase {
    +  /**
    +   * Provides meta information about this test case, such as test name.
    +   *
    +   * @return array
    +   *   An array of untranslated strings with the following keys:
    +   *   - name: An overview of what is tested by the class; for example, "User
    +   *     access rules".
    +   *   - description: One sentence describing the test, starting with a verb.
    +   *   - group: The human-readable name of the module ("Node", "Statistics"), or
    +   *     the human-readable name of the Drupal facility tested (e.g. "Form API"
    +   *     or "XML-RPC").
    +   */
    @@ -29,7 +41,10 @@ public static function getInfo() {
    +  /**
    +   * Specific sets up a for running functional and integration tests.
    +   */
    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleStringTest.php
    @@ -42,7 +44,10 @@ public static function getInfo() {
    +  /**
    +   * Specific sets up a for running functional and integration tests.
    +   */

    All of these should be just

    <?php
    /**
    * {@inheritdoc}
    */
    ?>
  6. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php
    @@ -32,7 +44,7 @@ public static function getInfo() {
    -  function testMachineNameLTR() {
    +  public  function testMachineNameLTR() {

    It seems you're introducing a double space here?!

  7. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php
    @@ -419,10 +432,9 @@ function getOverwritePoFile() {
    -   * Helper function that returns a .po file which strings will be marked
    -   * as customized.
    +   * Helper function that returns a .po file which strings will be marked as customized.

    Our standard is that the one-line description does not exceed 80 characters, so we'll need to shorten that.
    We can just remove the "Helper function that" part and start directly with "Returns..."

  8. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php
    @@ -29,8 +41,10 @@ public static function getInfo() {
    +   * Test file parsing.

    Should be "Test*s*".