Part of meta-issue #1518116: [meta] Make Core pass Coder Review

Remaining tasks:

  • (done) open issues to fix false negatives for coder listed below
  • (skipping this) Decide on #47 (to break arrays over 80 char).
  • keep rerolling as needed

We are not changing @param @return types or typehints in function signatures in this issue.

See #66 and #2288793: Add missing and fix some types in core docblocks and add some typehinting for locale module

Other issues:

#2271485: Move LocaleStringTest $only_translated = array('untranslated' => FALSE); out of accidental comment

Coder issues that helped reduce false errors that have been fixed while working on this issue

Things not fixed in this issue:

  • Replacing * Implements ... with {@inheritdoc}
  • .js files because #1778828: [policy, no patch] Update JS coding standards not done
  • .css files
  • rewording long comments to be (under) 80 chars (since that might change the meaning of the comments to be incorrect and makes it too difficult to review)

Review hints

To install code sniffer:

  • Install composer (the install composer instructions that are part of the drush ones might be nice https://github.com/drush-ops/drush/blob/master/README.md )
  • get phpcs (2.0 dev required to work with 8.x coder_sniffer), by for example: composer global require squizlabs/PHP_CodeSniffer:2.0.x-dev
  • get the 8.x version of coder (which has the Drupal sniffer rules), instructions on https://www.drupal.org/node/1419988 (which say to add "minimum-stability": "dev" to composer.json, and then composer global require drupal/coder:dev-8.x-2.x (we used to have to use the fork for the Coder meta, but changes to Coder 8.x seem to not make that necessary right now)
  • add the composer vendor bin to your path so you can run phpcs
  • (not currently needed) patch coder (to ignore type things, or to ignore rules that are broken)

Which should end up with something like this example this composer.json (or just use this and then run composer update)

{
    "require": {
        "squizlabs/PHP_CodeSniffer": "2.0.x-dev",
        "drupal/coder": "dev-8.x-2.x"
    },
    "minimum-stability": "dev"
}

run like:
phpcs --extensions=php,module,inc,install,test,profile,theme --standard=$path_to_sniffer_Drupal/coder-8.x/coder_sniffer/Drupal --warning-severity=0 -- core/modules/locale
or link the drupal standards like:
ln -s /Users/me/.composer/vendor/drupal/coder/coder_sniffer/Drupal /Users/me/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Drupal
and then run like:
phpcs --extensions=php,module,inc,install,test,profile,theme --standard=Drupal --warning-severity=0 -- core/modules/locale
which will ignore .js and css files
and not print out warnings (only prints errors)

Files: 
CommentFileSizeAuthor
#95 interdiff.1533250.94.95.txt1.62 KBYesCT
#95 drupal-make_locale_module_pass_coder_review-1533250-95.patch114.14 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,219 pass(es).
[ View ]
#94 drupal-make_locale_module_pass_coder_review-1533250-94.patch113.19 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,171 pass(es).
[ View ]
#92 interdiff.leftout.txt8.58 KBYesCT
#92 drupal-make_locale_module_pass_coder_review-1533250-92.patch115.73 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,424 pass(es).
[ View ]
#91 interdiff.1533250.90.91.txt639 bytesYesCT
#91 drupal-make_locale_module_pass_coder_review-1533250-91.patch121.28 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,252 pass(es).
[ View ]
#90 drupal-make_locale_module_pass_coder_review-1533250-90.patch120.92 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,160 pass(es).
[ View ]
#89 interdiff.1533250.88.89.txt1.12 KBYesCT
#89 drupal-make_locale_module_pass_coder_review-1533250-89.patch124.79 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,290 pass(es).
[ View ]
#87 interdiff.1533250.79.86.txt497 bytesYesCT
#87 drupal-make_locale_module_pass_coder_review-1533250-86.patch124.84 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,862 pass(es).
[ View ]
interdiff.1533250.79.84.txt497 bytesYesCT
drupal-make_locale_module_pass_coder_review-1533250-84.patch125.13 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,868 pass(es).
[ View ]
#80 interdiff.txt1.84 KBTravisCarden
#80 drupal-make_locale_module_pass_coder_review-1533250-79.patch125.13 KBTravisCarden
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,958 pass(es).
[ View ]
#71 drupal-make_locale_module_pass_coder_review-1533250-71.patch121.11 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,573 pass(es).
[ View ]
#70 sniffer_ignore_outstanding_issues-do-not-test.patch2.6 KBYesCT
#70 sniffer-ignore-type-things-do-not-test.patch4.02 KBYesCT
#69 interdiff.1533250.61.69.txt6.18 KBYesCT
#69 drupal-make_locale_module_pass_coder_review-1533250-69.patch112.66 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,573 pass(es).
[ View ]
#61 drupal-make_locale_module_pass_coder_review-1533250-61.patch116.76 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,549 pass(es).
[ View ]
#60 1533250-psr4-reroll-60.patch118.08 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,947 pass(es).
[ View ]
#58 1533250-psr4-reroll.patch117.13 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1533250-psr4-reroll.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#52 interdiff-1533250-51-52.txt616 bytesYesCT
#52 drupal-make_locale_module_pass_coder_review-1533250-52.patch119.13 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,621 pass(es).
[ View ]
#51 interdiff-1533250-50-51.txt4.75 KBYesCT
#51 drupal-make_locale_module_pass_coder_review-1533250-51.patch119.03 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,670 pass(es).
[ View ]
#50 interdiff-1533250-49-50.txt1.27 KBYesCT
#50 drupal-make_locale_module_pass_coder_review-1533250-50.patch116.11 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,582 pass(es).
[ View ]
#49 interdiff-1533250-42-49.txt11.79 KBYesCT
#49 drupal-make_locale_module_pass_coder_review-1533250-49.patch115.12 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,681 pass(es).
[ View ]
#42 interdiff-1533250-40-42.txt882 bytesYesCT
#42 drupal-make_locale_module_pass_coder_review-1533250-42.patch107.63 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,579 pass(es).
[ View ]
#40 drupal-make_locale_module_pass_coder_review-1533250-40.patch107.63 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,579 pass(es).
[ View ]
#40 interdiff-35-40.txt1017 bytesfran seva
#35 drupal-make_locale_module_pass_coder_review-1533250-35.patch107.67 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,213 pass(es).
[ View ]
#35 interdiff-33-35.txt9.05 KBfran seva
#33 drupal-make_locale_module_pass_coder_review-1533250-33.patch111.55 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,175 pass(es).
[ View ]
#28 drupal-make_locale_module_pass_coder_review-1533250-28.patch112.45 KBfran seva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,084 pass(es).
[ View ]
#26 drupal-make_locale_module_pass_coder_review-1533250-26.patch108.67 KBfran seva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_locale_module_pass_coder_review-1533250-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#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

NROTC_Webmaster’s picture

Status:Active» Postponed
Lars Toomre’s picture

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.

Lars Toomre’s picture

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.

TravisCarden’s picture

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!

TravisCarden’s picture

Issue summary:View changes
Status:Postponed» Active
sriharsha.uppuluri’s picture

Assigned:Unassigned» sriharsha.uppuluri

I'm starting on coder review

sriharsha.uppuluri’s picture

Started working on the coder review

sriharsha.uppuluri’s picture

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.

sriharsha.uppuluri’s picture

Assigned:sriharsha.uppuluri» Unassigned
fran seva’s picture

Assigned:Unassigned» fran seva

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

fran seva’s picture

StatusFileSize
new746.64 KB

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

fran seva’s picture

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

TravisCarden’s picture

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!

fran seva’s picture

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

fran seva’s picture

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
--------------------------------------------------------------------------------

fran seva’s picture

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

fran seva’s picture

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.

drupalninja99’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

fran seva’s picture

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.

fran seva’s picture

I'm still working on it

fran seva’s picture

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!!

tstoeckler’s picture

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*".

fran seva’s picture

I'm rerolling...

fran seva’s picture

Status:Needs work» Needs review
StatusFileSize
new108.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_locale_module_pass_coder_review-1533250-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@tstoeckler I have applied the changes and after review the code the followings error are showed by phpcs:

In LocaleLookupTest.php, after revert the changes the current phpcs for Drupal return bad indentation.

  • phpcs: Line indented incorrectly; expected 4 spaces, found 6
  • phpcs: Line indented incorrectly; expected 37 spaces, found 8
  • phpcs: Line indented incorrectly; expected 39 spaces, found 10
  • phpcs: Line indented incorrectly; expected 6 spaces, found 8
  • phpcs: Line indented incorrectly; expected 4 spaces, found 6
  • phpcs: Line indented incorrectly; expected 37 spaces, found 8
  • phpcs: Line indented incorrectly; expected 39 spaces, found 10
  • phpcs: Case breaking statements must be followed by a single blank line
  • phpcs: Line indented incorrectly; expected 39 spaces, found 19
  • phpcs: Line indented incorrectly; expected 39 spaces, found 10
  • phpcs: Case breaking statements must be followed by a single blank line
  • phpcs: Line indented incorrectly; expected 39 spaces, found 19
  • phpcs: Line indented incorrectly; expected 39 spaces, found 10
  • phpcs: Line indented incorrectly; expected 39 spaces, found 19

In TranslateFormBase.php there is an error that I don't know how fix it (Should be easy but...phpcs is telling me that I'm a looser :( ) The code are in [1]

  • phpcs: Return comment must be on the next line

Also, I have made the changes to fix the error with the method locale_translation_batch_status_check

  • Change the comment removing "Optional"
  • Change the function input parameter $options = array() to array $options to avoid the phpcs error.

[1]

/**
   * Builds an array out of search criteria specified in request variables.
   *
   * @param bool $reset
   *   If the list of values should be reset.
   *
   * @return array $filter_values
   *   The filter values.
   */

Status:Needs review» Needs work

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

fran seva’s picture

StatusFileSize
new112.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,084 pass(es).
[ View ]

Fix the patch...

fran seva’s picture

Status:Needs work» Needs review
tstoeckler’s picture

Status:Needs review» Needs work

@franSeva Thanks a lot for your hard work on this. I found a few more things that need fixing, sadly. Can you give this another shot?

  1. +++ b/core/modules/locale/lib/Drupal/locale/StringInterface.php
    @@ -26,7 +26,8 @@ public function getId();
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -44,7 +45,8 @@ public function getVersion();
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -62,7 +64,8 @@ public function getString();
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -82,7 +85,8 @@ public function getPlurals();
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -100,7 +104,8 @@ public function getStorage();
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -136,7 +141,8 @@ public function isTranslation();
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -181,16 +187,17 @@ public function getLocations($check_only = FALSE);
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -201,7 +208,8 @@ public function hasLocation($type, $name);
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    @@ -211,7 +219,8 @@ public function save();
    -   * @return $this
    +   * @return Object
    +   *   $this Object

    These changes are incorrect. Please revert them.

  2. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationUiTest.php
    @@ -23,7 +23,18 @@ class LocaleTranslationUiTest 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").

    This should be just {@inheritdoc}.

  3. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationUiTest.php
    @@ -204,11 +214,10 @@ function testStringTranslation() {
    -   * Adds a language and checks that the JavaScript translation files are
    -   * properly created and rebuilt on deletion.
    ...
    +   * Adds a language and checks that the JavaScript translation files are properly created and rebuilt on deletion.

    Our standard dictates that the one-line description must not exceed 80 characters, so this needs to be shortened in some way. I would suggest

    Tests that JavaScript translation files are properly created and rebuilt.
  4. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateBase.php
    @@ -177,28 +180,27 @@ protected function setTranslationFiles() {
    -   * Setup existing translations in the database and set up the status of
    -   * existing translations.
    +   * Setup existing translations in the database and set up the status of existing translations.

    Agian, this should be shortened. Maybe just

    Sets up existing translations in the database.
  5. +++ b/core/modules/locale/locale.api.php
    @@ -109,7 +109,7 @@
    - * @see locale_translation_project_list().
    + * @see locale_translation_project_list

    The parentheses should be kept here.

  6. +++ b/core/modules/locale/locale.compare.inc
    @@ -211,8 +211,8 @@ function locale_translation_default_translation_server() {
    + * // @todo Return batch or NULL

    There should be a newline before this line and the "//" should be removed.

  7. +++ b/core/modules/locale/locale.compare.inc
    @@ -282,8 +282,7 @@ function locale_translation_batch_status_build($projects = array(), $langcodes =
    - * Helper function to construct batch operations checking remote translation
    - * status.
    + * Helper function to construct batch operations checking remote translation status.

    Again, this needs to be shortened. Perhaps: "Concstructs batch operations..."

  8. +++ b/core/modules/locale/locale.module
    @@ -1107,8 +1110,7 @@ function _locale_strip_quotes($string) {
    - * Parses a JavaScript file, extracts strings wrapped in Drupal.t() and
    - * Drupal.formatPlural() and inserts them into the database.
    + * Parses a JavaScript file, extracts strings wrapped in Drupal.t() and Drupal.formatPlural() and inserts them into the database.

    Suggestion: "Parses a JavaScript file for translatable string and saves them to the database."

  9. +++ b/core/modules/locale/locale.module
    @@ -1242,7 +1244,7 @@ function _locale_invalidate_js($langcode = NULL) {
    + * Re-Creates the JavaScript translation file for a language.

    I think "Re-creates" should have a lowercase "c".

  10. +++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleTranslationTest.php
    @@ -32,6 +32,18 @@ class LocaleTranslationTest extends UnitTestCase {
    +   * 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").

    This should be just "{@inheritdoc}"

  11. +++ b/core/modules/locale/tests/Drupal/locale/Tests/Menu/LocaleLocalTasksTest.php
    @@ -17,6 +17,18 @@
    +   * 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").

    This should be "{@inheritdoc}"

fran seva’s picture

@tstoeckler sure. I will change and review it.
Thanks for review!!!

fran seva’s picture

The patch #28 doesn't apply, needs reroll.
I'll do it.

fran seva’s picture

StatusFileSize
new111.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,175 pass(es).
[ View ]

Re-roll patch:
- Locale.module has been modified. Hook_help has change and now use route_name
- locale.bulk.inc has remove code
attach the patch

fran seva’s picture

Status:Needs work» Needs review
fran seva’s picture

StatusFileSize
new9.05 KB
new107.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,213 pass(es).
[ View ]

@tstoeckler I have made all changes but phpcs still shows errors in point 1.
phpcs: @return data type must not contain "$"
phpcs: Missing comment for @return statement

May be that my phpcs is outdate, I'm going to check it.

Attach the patch and the interdiff!! XD

YesCT’s picture

https://drupal.org/node/1354#types
says that $this can be a type,
so I guess we need to update phpcs rules...
the meta #1518116: [meta] Make Core pass Coder Review has a "blockers" section.
maybe we need something in the coder project issue queue like #1805544: Modify sniff for "Implements hook_foo_BAR_ID_bar() for xyz_bar()", but for updating the rule to allow $this in @return?

fran seva’s picture

@yesct I have checked the last coder module and currently don't allow "$" in @return comments. So, as you said, would be needed an update in phpcs rules.


if (strpos($return->getValue(), '$') !== false) {
                    $error = '@return data type must not contain "$"';
                    $this->currentFile->addError($error, $errorPos, '$InReturnType');
}
tstoeckler’s picture

Status:Needs review» Needs work

Again, great work @fran seva!
Sadly I couldn't yet mark this RTBC, but there are two super small errors in the patch:

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

    Seems like this removes the blank line between the two methods. That should be kept.

  2. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationUiTest.php
    @@ -23,7 +23,9 @@ class LocaleTranslationUiTest extends WebTestBase {
    -
    +  /**
    +   * {@inheritdoc}
    +   */

    Here also the blank line should be kept.

It would be great if you could fix that, then we can set this RTBC :-)

fran seva’s picture

@tstoeckler I didn't seen the llines in the interdiff :(
I'm on it :)
Again, thanks for review!!!

fran seva’s picture

Status:Needs work» Needs review
StatusFileSize
new1017 bytes
new107.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,579 pass(es).
[ View ]

here is the patch and its interdiff :)

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

Yay. fran Seva, you rock!

Thanks for your persistence.

YesCT’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new107.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,579 pass(es).
[ View ]
new882 bytes
  1. +++ b/core/modules/locale/lib/Drupal/locale/Form/LocaleSettingsForm.php
    @@ -108,22 +108,25 @@ public function submitForm(array &$form, array &$form_state) {
             break;
    +
           case LOCALE_TRANSLATION_OVERWRITE_NONE:
             $config
               ->set('translation.overwrite_customized', FALSE)
               ->set('translation.overwrite_not_customized', FALSE)
               ->save();
             break;
    +
         }

    adding this blank line before the end of the case } seemed weird, so I checked https://drupal.org/coding-standards#controlstruct
    and also installed and ran the sniffer according to the summary in #1518116: [meta] Make Core pass Coder Review

    still passes without the blank line before the switch closing }.

  2. +++ b/core/modules/locale/lib/Drupal/locale/Form/LocaleSettingsForm.php
    @@ -108,22 +108,25 @@ public function submitForm(array &$form, array &$form_state) {
    -    // Invalidate the cached translation status when the configuration setting of
    -    // 'use_source' changes.
    +    // Invalidate the cached translation status when the configuration
    +    // setting of 'use_source' changes.

    this looks like it wrapped too soon.

    https://drupal.org/coding-standards/docs#drupal

    "lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over..."

    so re-wrapped this.

    still passes coder review.

    maybe we could look and see if coder has an issue to check that "as close to 80" rule.

I might look at this some more later.

YesCT’s picture

@fran seva can you open an Coder issue for #37 and link it here in the issue summary?

(maybe also for #42 2.)

fran seva’s picture

@YesCT thanks for review and the patch.
About #43 I'm on it

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

...I'm a bit annoyed at @YesCT having to show off that she can be even more nitpicky than me, but the patch looks good!

(...I'm kidding of course :-))

fran seva’s picture

I have created the issue [1] but I'm not sure about its category and version :S
Could anyone review it?

[1] https://drupal.org/node/2270551

YesCT’s picture

Are we not fixing errors like:
FILE: ...rupal2/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
184 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line

?

webchick’s picture

Status:Reviewed & tested by the community» Needs review
YesCT’s picture

Issue summary:View changes
StatusFileSize
new115.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,681 pass(es).
[ View ]
new11.79 KB

1.
Reviewing the patch a bit more into it,
found:

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.php
@@ -56,27 +56,27 @@ class LocaleConfigManager extends TypedConfigManager {
+    $this->installStorage  = $install_storage;

oops a double space.

fixed that.

2. So I ran
/Users/ctheys/.composer/vendor/bin/phpcs --standard=/Users/ctheys/.composer/vendor/drupal/coder/coder_sniffer/Drupal core/modules/locale

And found more things:

Patch fixes many more

2.a.
A comma should follow the last multiline array item.
2.b.
WARNING | Line exceeds 80 characters; contains ...
2.c.
Case breaking statements must be followed by a single blank
| | line

I checked and these were fixed in other places in the patch already, so I could not see why not to fix these.

========
Updated the issue summary with what I think a committer would want to know about what is fixed and what isn't.

--
Next:
Missing parameter type at position ...

YesCT’s picture

StatusFileSize
new116.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,582 pass(es).
[ View ]
new1.27 KB

oh, a couple of little fixes

* @return array $filter_values
was interesting
cause the error was
124 | ERROR | Return comment must be on the next line
and I was like: it is!
But there is a var name. :) took that off.

YesCT’s picture

StatusFileSize
new119.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,670 pass(es).
[ View ]
new4.75 KB

Now
Missing parameter type at position

YesCT’s picture

Issue summary:View changes
StatusFileSize
new119.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,621 pass(es).
[ View ]
new616 bytes
+++ b/core/modules/locale/locale.translation.inc
@@ -41,7 +41,7 @@
- *  @params array $project_names
+ * @params array $project_names
  *    Array of names of the projects to get.

correcting the indent here means we have to fix the line below also.

it is also
@param
not
@params

------
that ends reading through the whole patch and running coder on it.
ok. I'm done for a bit. :)

Now we just decide on #47 I guess.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

We don't enforce #47 very strictly throughout core and I always under the impression that that's merely a suggestion not a strict rule.

Interdiffs look good.

fran seva’s picture

@tstoeckler in comment #53 you mean that in the Travis Garden's fork branch issue-1518116 those kind of errors are not being checked?
I ask because when I ran the phpcs the error reported by @YesCT didn't appear and I wanna be sure that I'm using the correct coder version.

@YesCT thanks for your review and the corrections in the issue #2270551: Modify sniff to allow $ (this) in @return

tstoeckler’s picture

Re @fran seva: I didn't run phpcs at all, I just reviewed the patch manually, so I can't really say anything about that.

fran seva’s picture

Assigned:fran seva» Unassigned
xjm’s picture

StatusFileSize
new117.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1533250-psr4-reroll.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1533250-psr4-reroll.patch, failed testing.

fran seva’s picture

Status:Needs work» Needs review
StatusFileSize
new118.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,947 pass(es).
[ View ]

@xjm I have tried the PSR-4 reroll :)
I don't attach the interdiff. Is necessary in a reroll?

YesCT’s picture

StatusFileSize
new116.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,549 pass(es).
[ View ]

@fran seva interdiffs are tricky on rerolls. Sometimes I copy and paste the conflicts and how I resolve them. But the only way I really know how to review them is to do my own reroll and them compare. ;)

-----

I resolved the conflicts differently.

mine are on the left (<)

1.

< - *   contain a language parameter
< - *   (other than LanguageInterface::LANGCODE_NOT_SPECIFIED). This is used as
< - *   the language of the import.
< + *   contain a language parameter (other than
< + *   LanguageInterface::LANGCODE_NOT_SPECIFIED). This is used as the language of
< + *   the import.
---
> - *   contain a language parameter (other than Language::LANGCODE_NOT_SPECIFIED). This
> - *   is used as the language of the import.
> + *   contain a language parameter (other than Language::LANGCODE_NOT_SPECIFIED).
> + *   This is used as the language of the import.

I wrapped this so that (other than... was on the line

2.
and

< -      $output = '<p>' . t('Translation files are automatically downloaded and imported when <a title="Languages" href="!language">languages</a> are added, or when modules or themes are enabled.', array('!language' => \Drupal::url('language.admin_overview'))). '</p>';
< +      $output = '<p>' . t('Translation files are automatically downloaded and imported when <a title="Languages" href="!language">languages</a> are added, or when modules or themes are enabled.', array('!language' => \Drupal::url('language.admin_overview'))) . '</p>';

I did this which was in the patch before, of adding the space before the .
which seems to be missing from #60

3.
and I'm not sure why patch in #60 deleted SystemLocalTasksTest
but my patch leaves this in.

---
no interdiff since #60 didn't apply already.

fran seva’s picture

@YesCT thanks for your tip on reroll and how review it.

About:
1. OK
2. I didn't see the space. I remember this was a conflict in the reroll and I check the text and the Drupal::url but not the space
3. Following the steps in [#424758] this file was check to be deleted so I create the patch with git -C -M to insert the deletion.
I did it because I read this in the reroll psr4 steps:

Remember, this script will add and remove files without updating the git index, so you will need to add and remove files with git add.

@YesCT thanks again for your review. I'm going to try to make the patch again to check point 3. and understand what I did wrong.

YesCT’s picture

hm.

re 3.

running php core/scripts/switch-psr4.sh on 8.x head deletes a file.
core/modules/system/tests/Drupal/system/Tests/Menu/SystemLocalTasksTest.php

Since that is a problem in head, we shouldn't be messing with it here.
Made a separate issue #2286241: Move stray SystemLocalTasksTest to PSR-4 location

fran seva’s picture

@YesCT I'll wait till this #2286241: Move stray SystemLocalTasksTest to PSR-4 location will be fixed.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the explicit merge-interdiff (or however to call that). That is helpful. Looks great.

TravisCarden’s picture

Status:Reviewed & tested by the community» Needs work

Sorry to come late to the game with this feedback, but type hinting doxygen directives (@param and @return) has been excluded from this initiative and moved to #1800046: [META] Add missing type hinting to core docblocks out of respect for committers who feel that those changes make these patches painful for them to review. If we can just remove those from this patch, the committers will appreciate it, and we'll be more likely to get this committed on the first try. Note: If you're following the instructions on the meta issue for using our Coder spork, then Sniffer won't complain about missing type hinting.

YesCT’s picture

Using the 8.x branch of coder eliminates some false positive errors.
Not the github fork.

Maybe we need a fork of the 8.x stuff? or directions for how to silence the type hinting stuff?

I'm ok with taking it out, and making a separate issue... if that is what committers want.

YesCT’s picture

Ok. I'll split this into two issues.

YesCT’s picture

Status:Needs work» Needs review
StatusFileSize
new112.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,573 pass(es).
[ View ]
new6.18 KB

I have to double check if I did all the diffs correctly.
And need to open another issue for the @param @return and typehint changes.
And need to provide instructions for reviewing (might want to combine the fork which ignores the types, and the 8.x branch of code sniffer which has eliminates 8.x false positive errors).

But here are the files for now. I'll be back at this in just a bit.

YesCT’s picture

updated issue summary with more separate coder sniffer issues,
next, a new patch (and interdiff) with more fixes found using the 8.x

YesCT’s picture

StatusFileSize
new121.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,573 pass(es).
[ View ]
new11.87 KB

done for now. let's get some feedback as to if this is reviewable. (see review hints in the issue summary)
interdiff is those things I found while running 8.x coder_sniffer on them.

tstoeckler’s picture

+++ b/core/modules/locale/locale.batch.inc
@@ -25,7 +25,7 @@
- *   An array with options that can have the following elements:
+ *   Optional, an array with options that can have the following elements:

@@ -33,7 +33,7 @@
-function locale_translation_batch_status_check($project, $langcode, array $options, &$context) {
+function locale_translation_batch_status_check($project, $langcode, $options = array(), &$context) {

The current code is just wrong, let's please not re-introduce that code just because of issue queue madness.

  1. +++ b/core/modules/locale/locale.module
    @@ -108,14 +108,12 @@
    + * UI option for override of only non-customized existing translations.
    ...
    const LOCALE_TRANSLATION_OVERWRITE_NON_CUSTOMIZED = 'non_customized';
    ...
    + * UI option for don't override of existing translations.
    ...
    const LOCALE_TRANSLATION_OVERWRITE_NONE = 'none';

    I think we generally try to avoid "don't" style abbreviations. Also I think the sentence-flow could be improved. Not a native speaker, but shouldn't be "for overriding"? Also the constants use "overwrite" instead of "override", that looks very strange. Not sure which one is correct, though.

  2. +++ b/core/modules/locale/locale.translation.inc
    @@ -158,7 +161,7 @@ function locale_translation_build_sources($projects = array(), $langcodes = arra
    - * @return stdClass
    + * @return object

    As far as I know \stdClass is actually preferred over object, but I might be mistaken.

YesCT’s picture

Status:Needs review» Needs work

I promise to do the issue (#2288793: Add missing and fix some types in core docblocks and add some typehinting for locale module) that actually changes @param and @return and type hinting. But for this coding standards one, I'd rather get this in, and @jhodgdon in irc confirmed @TravisCarden 's #66 request.

I'll fix 1.

And 2. https://www.drupal.org/node/1354#types
object (NOT "stdClass")

-
working on this now.

YesCT’s picture

Status:Needs work» Needs review
StatusFileSize
new121.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,953 pass(es).
[ View ]

just a reroll.

3 conflicts, but they were simple. In 2, just redid the patch changes on the new head lines, and in 1 it was just a context line that changed in head, and kept head. :)

YesCT’s picture

Issue tags:-Novice
StatusFileSize
new121.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,881 pass(es).
[ View ]
new465 bytes

here is that re-wording.

next, re-run the sniffer on this to see if this is covering all it should.

nothing really novice at this point. :)

YesCT’s picture

I reran the sniffer, and all the errors it reports are due to the sniff issues (see the list of related issues) or are things we are not going to fix in this issue.

I think this is ready again.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

I'm very, very frustrated with the process of this issue, but the patch itself is RTBC.

Thanks for your tireless effort @YesCT and @fran seva!!!

TravisCarden’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new125.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,949 pass(es).
[ View ]
new7.55 KB
new17.06 KB

@YesCT requested my eyes on this patch. Everyone's done a great job! There are just a few little things left. I've gone ahead and fixed them myself. (Attached.) It just all stuff.

@YesCT, are you using the Coder 8.x branch? (I notice some of the related issues filed against Coder are against it.) I haven't had the time to get it working, so I'm still using my spork as described on the meta. If we've figured out how to use 8.x, it would be good to update the instructions there. Since there remains a question, I'm attaching a bash history of my entire review process. (See issue-1533250-review-history.txt attached.)

tstoeckler’s picture

Status:Needs review» Needs work

The additional fixes look good, thanks. Sadly, this is no longer RTBC:

  1. +++ b/core/modules/locale/src/Tests/LocaleStringTest.php
    @@ -156,7 +156,8 @@ public function testStringSearchAPI() {
    +    // Load all translations. For next queries we'll be loading only translated
    +    // strings. $only_translated = array('untranslated' => FALSE);

    Please revert this. This is being handled in #2271485: Move LocaleStringTest $only_translated = array('untranslated' => FALSE); out of accidental comment.

  2. +++ b/core/modules/locale/src/Tests/LocaleTranslateStringTourTest.php
    @@ -52,7 +52,7 @@ protected function setUp() {
    -    // Add Another Language so there is no missing form items.
    +    // Add Another Language so there are no missing form items.

    Since we're changing this anyway (now), let's lowercase 'Another' and 'Language'.

TravisCarden’s picture

Status:Needs work» Needs review
StatusFileSize
new125.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,958 pass(es).
[ View ]
new1.84 KB

Thank you, @tstoeckler. Here you go.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the quick turnaround. Let's see if my 6th RTBC will be the last...

jhodgdon’s picture

I hate these patches... I feel obligated to review them carefully before I commit them. So of course I find an error or two and they are so huge, you don't want to make them go through yet another cycle of patch/review.

Still, in locale.module:

/**
- * (Re-)Creates the JavaScript translation file for a language.
+ * Re-creates the JavaScript translation file for a language.
  *

I'm assuming the previous documentation here was saying that it either creates or re-creates the file, right? The new docs change the meaning. Did someone check to see which was correct? If not I would just leave this as is.

I'm also not sure about all the explicit adds of "public" to methods on various classes. Difficult for me to evaluate whether they are correct or not.

So... at that point in the patch (I got about 1/3 of the way through), I gave up... sorry.

kim.pepper’s picture

From the PHP docs: http://www.php.net/manual/en/language.oop5.visibility.php

Method Visibility
Class methods may be defined as public, private, or protected. Methods declared without any explicit visibility keyword are defined as public.

YesCT’s picture

Status:Reviewed & tested by the community» Needs review
kim.pepper’s picture

I'm also not sure about all the explicit adds of "public" to methods on various classes. Difficult for me to evaluate whether they are correct or not.

I think we should be adding explicit public visibility to methods. I was pointing out that we are safe to do so, where no visibility modifier is provided.

YesCT’s picture

StatusFileSize
new124.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,862 pass(es).
[ View ]
new497 bytes

oops. #84 was messed up.
Here is what it should have been.

@jhodgdon thanks for getting that far. I checked and (Re-) is correct, so I put that back in.

YesCT’s picture

reroll. auto 3 way merge. no conflicts.

YesCT’s picture

StatusFileSize
new124.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,290 pass(es).
[ View ]
new1.12 KB

1.

+++ b/core/modules/locale/locale.module
@@ -98,7 +98,7 @@
- * The number of seconds that the translations status entry should be considered.
+ * The number of seconds the translations status entry should be considered.

this was strange to start with.

usages:

  // If the status was updated recently we can immediately start fetching the
  // translation updates. If the status is expired we clear it an run a batch to
  // update the status and then fetch the translation updates.
  $last_checked = \Drupal::state()->get('locale.translation_last_checked');
  if ($last_checked < REQUEST_TIME - LOCALE_TRANSLATION_STATUS_TTL) {
    locale_translation_clear_status();
    $batch = locale_translation_batch_update_build(array(), $langcodes, $options);
    batch_set($batch);
  }
  else {
    $batch = locale_translation_batch_fetch_build($projects, $langcodes, $options);
    batch_set($batch);
  }

So how about:

* The number of seconds, after which the translations status entry expires.

2.
also added a period to a @todo sentence.

---
read through the whole thing again, still looks good to me.

YesCT’s picture

StatusFileSize
new120.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,160 pass(es).
[ View ]

reroll for #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

Some things to check about the moving of the getInfo descriptions to the one line summary on the classes.
patch coming.

YesCT’s picture

StatusFileSize
new121.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,252 pass(es).
[ View ]
new639 bytes

tried to reword the one line summary for 80 chars.

I'm currently using
in composer.json

{
"require": {
"drush/drush": "dev-master",
"squizlabs/PHP_CodeSniffer": "2.0.x-dev",
"drupal/coder": "dev-8.x-2.x"
},
"minimum-stability": "dev"
}

run composer update from ~/.composer to get the latest 8.x fixes to coder

linked the standard
ln -s /Users/me/.composer/vendor/drupal/coder/coder_sniffer/Drupal /Users/me/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Drupal

and run like
phpcs -- --standard=Drupal --extensions=php,module,inc,install,test,profile,theme core/modules/locale

YesCT’s picture

StatusFileSize
new115.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,424 pass(es).
[ View ]
new8.58 KB

ok. here is 91, but simplified, to take out any comment changes or anything that might make it more involved to review (like rewording comments).

If someone reviews this, please specify if you are reviewing 91 or 92
Attaching what I left out of 92.

herom’s picture

Status:Needs review» Needs work

reviewed patch #92.
one relevant error from coder:

FILE: ...links/core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
27 | ERROR | Visibility must be declared on method "testFileParsing"
--------------------------------------------------------------------------------

two cases with multiple-spacing in the lines. should they be fixed here?

locale.install:126:      'lid'     => array('lid'),
locale.bulk.inc:77: *   Language codes from which to  get the translation files and history.
                                                    ^^
YesCT’s picture

YesCT’s picture

StatusFileSize
new114.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,219 pass(es).
[ View ]
new1.62 KB

fixes from feedback in #93.

re-ran the sniffer, and looks like got the actual errors that we are going to address here

herom’s picture

Status:Needs review» Reviewed & tested by the community

I didn't find any more issues.

YesCT’s picture

Title:Make locale module pass Coder Review» Many coding standards clean-ups in locale module
Issue summary:View changes

the scope of this is smaller than actually making it pass Coder. retitling.

several errors we were ignoring have had their rules updated in Coder. (Thanks!)

So taking those notes out of the issue summary, and just listing the related issues.

updated review/install instructions. I'm not sure why we dont need the fork that ignores the @param and @return rule.. but Coder 8.x seems to be ok without that fork (for this issue)

YesCT’s picture

Issue summary:View changes

html

YesCT’s picture

+++ b/core/modules/locale/src/TranslationsStream.php
@@ -17,17 +17,18 @@
-   * Implements Drupal\Core\StreamWrapper\LocalStream::getDirectoryPath()
+   * {@inheritdoc}
...
-   * Implements Drupal\Core\StreamWrapper\StreamWrapperInterface::getExternalUrl().
+   * {@inheritdoc}

if we have to reroll this again. after the reroll, we could take out these two changes (since they require more review work). Leaving in for now.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 12c2fbb and pushed to 8.x. Thanks!

  • alexpott committed 12c2fbb on 8.x
    Issue #1533250 by YesCT, fran seva, TravisCarden, xjm, sriharsha....

Status:Fixed» Closed (fixed)

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