Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NROTC_Webmaster’s picture

lotyrin’s picture

Assigned: Unassigned » lotyrin
lotyrin’s picture

Status: Active » Needs review
FileSize
891 bytes

This one was easy. No doc changes.

xjm’s picture

Let's update the issue here with:

  • Whether this directory either passes or fails on the Drupal 8 branch test code review tab. If it fails, also note whether the failures are itemized on that page.
  • Whether the directory has been tested with Drupal Code Sniffer with the patch applied, whether it passes or fails, and what settings were used.
  • Whether the directory has been tested with the D8 sandbox of Coder with the patch applied, whether it passes or fails, and what settings were used.

In each case note any failures that were not corrected, and why.

lotyrin’s picture

Assigned: lotyrin » Unassigned

Unassigning because I'm not sure I'll have time for these issues and I want to make sure people know they can jump in without stepping on my toes.

TravisCarden’s picture

Status: Needs review » 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
Jalandhar’s picture

Assigned: Unassigned » Jalandhar

Hi,

I have started working on it.

Jalandhar’s picture

Status: Active » Needs review
FileSize
32.21 KB

Hi,

I have done all coder errors related to rdf module except the below 1 error. Please suggest me on how can I do the below one.

FILE: ...ewdrupal8/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
30 | ERROR | Class property $base_uri should use lowerCamel naming without
| | underscores
--------------------------------------------------------------------------------

Attaching the patch for remaining all coder errors.

Status: Needs review » Needs work

The last submitted patch, 9: drupal-make_rdf_module_pass_coder_review-1533380-9.patch, failed testing.

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
33.97 KB

Another try at the patch.

scor’s picture

Status: Needs review » Needs work

30 | ERROR | Class property $base_uri should use lowerCamel naming without
| | underscores

That's fine by me, there is no reason to have it called $base_uri and not $baseUri, so feel free to make that change on line 30 and in the rest of that file.

The last submitted patch, 11: drupal-make_rdf_module_pass_coder_review-1533380-11.patch, failed testing.

scor’s picture

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
@@ -460,8 +472,7 @@ protected function assertRdfaNodeCommentProperties($graph) {
       // There is an extra carriage return in the when parsing comments as
       // output by Bartik, so it must be added to the expected value.
-      'value' => "$text
-",
+      'value' => "$text",

We need to keep this extra CR here. See comment explaining why we have an extra CR here.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php
@@ -149,14 +155,14 @@ public function testCommentRdfaMarkup() {
     $graph = new \EasyRdf_Graph();
     $parser->parse($graph, $this->drupalGet('node/' . $this->node->id()), 'rdfa', $this->base_uri);
-    $this->_testBasicCommentRdfaMarkup($graph, $comment1);
+    $this->testBasicCommentRdfaMarkup($graph, $comment1);

Renaming those to testSomething() will trigger the test suite to try to run those as indivual tests. They should be renamed to something like runTestSomething()

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
33.7 KB

Hi Scor.

Thanks for your suggestions. I have made the changes that you have said in the comment #14.

Regarding extra CR, coder is returning error if I keep extra CR as

FILE: ...ww/drupal/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
476 | ERROR | Array indentation error, expected 6 spaces but found 0
--------------------------------------------------------------------------------

Done the remaining errors and attaching the patch.

Status: Needs review » Needs work

The last submitted patch, 15: drupal-make_rdf_module_pass_coder_review-1533380-15.patch, failed testing.

dman’s picture

Would

'value' => "$text\n",

meet the requirement?

Jalandhar’s picture

Hi Scor,

The failed test cases in comment #15 is in image and view module. can you please suggest me on how I can fix it?

dman’s picture

@jalandhar It could be that the testbot is just a bit sick right now. I've seen it happen a few times today, and the system is working really hard.

"The test did not complete due to a fatal error." appears to be something with the testing system, not the code.
https://drupal.org/project/issues/search?issue_tags=Random%20test%20failure

Generally it may be worth waiting a few hours then queuing for re-test.

dman’s picture

Status: Needs work » Needs review
scor’s picture

Status: Needs review » Needs work
Issue tags: +RDF code sprint
  1. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/EmailFieldRdfaTest.php
    @@ -30,7 +33,9 @@ public static function getInfo() {
       }
    -
    +  /**
    +   * {@inheritdoc}
    +   */
       public function setUp() {
    

    I would expect to see a empty line above a docs block.

  2. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaDatatypeCallbackTest.php
    @@ -59,6 +65,4 @@ public function testDefaultFormatter() {
         $this->assertFormatterRdfa('text_default', 'http://schema.org/interactionCount', 'foo' . $this->test_value);
       }
    -
     }
    

    There is usually an empty line after the last method of a class, see https://drupal.org/node/608152#indenting

  3. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
    @@ -409,7 +418,8 @@ protected function assertRdfaArticleProperties($graph, $message_prefix) {
    -    //$this->assertEqual($graph->type($this->termUri), 'schema:Thing', 'Tag type was found (schema:Thing).');
    +    // $this->assertEqual($graph->type($this->termUri), 'schema:Thing',
    +    // 'Tag type was found (schema:Thing).');
    

    I find this much harder to read. Is that something coder review is complaining about? This is a commented piece of code pending a todo, not an actual comment.

  4. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
    @@ -418,7 +428,9 @@ protected function assertRdfaArticleProperties($graph, $message_prefix) {
    -    //$this->assertTrue($graph->hasProperty($this->termUri, 'http://schema.org/name', $expected_value), "$message_prefix name was found (schema:name).");
    +    // $this->assertTrue($graph->hasProperty($this->termUri,
    +    // 'http://schema.org/name', $expected_value),
    +    // "$message_prefix name was found (schema:name).");
    

    ditto. and there are more further.

scor’s picture

@dman re #17, good idea, maybe something like this:

'value' => "$text\n      ",
dman’s picture

3,4
Yes, coder review will complain about comments longer than 80 cols.
I'm cleaning up some of my own code tonight, and yeah, my temporarily commented out code blocks count as misformatted comments.
It also complains about indenting if we try to keep the old code layout.
And yes, it does become harder to read if we comply.

Either have to remove temporary cruft altogether, make an exception for those cases, or scrunch the code.

Sachini’s picture

I made some modifications to go with Object Oriented coding standards.

My patch contains changes by jalandhar too, since I applied patch drupal-make_rdf_module_pass_coder_review-1533380-15.patch, before starting

Jalandhar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 24: drupal-make_RDF_module_pass_coder_review-1533380-23.patch, failed testing.

Jalandhar’s picture

Hi Scor and damn,

Regarding 3 and 4 of comment #21, coder review is giving error that longer than 80 cols. can you please suggest me on how I can proceed about it so that I can do all changes of comment #21 and update patch.

scor’s picture

  1. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php
    @@ -43,7 +49,7 @@ public function setUp() {
         $this->setCommentForm(TRUE);
    -    $this->setCommentSubject(TRUE);
    +        $this->setCommentSubject(TRUE);
         $this->setCommentSettings('comment_default_mode', COMMENT_MODE_THREADED, 'Comment paging changed.');
    

    indentation issue.

  2. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php
    @@ -149,14 +155,14 @@ public function testCommentRdfaMarkup() {
    -    $this->_testBasicCommentRdfaMarkup($graph, $comment1);
    +    $this->runtestBasicCommentRdfaMarkup($graph, $comment1);
    

    It should be runTestBasicCommentRdfaMarkup() instead of runtestBasicCommentRdfaMarkup(). runTest should be spellt with uppercase T since Test is the second word in that CamelCase method name.

  3. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/RdfaAttributesTest.php
    @@ -32,19 +35,19 @@ public static function getInfo() {
    -    $this->_testAttributes($expected_attributes, $mapping);
    +    $this->testAttributes($expected_attributes, $mapping);
    

    the method should be runTestAttributes() like it was in #15.

  4. +++ b/core/modules/rdf/rdf.api.php
    @@ -36,6 +36,7 @@ function hook_rdf_namespaces() {
    +
    

    no need for a new line here, this is a regular function in a .module file (not a class).

  5. +++ b/core/modules/rdf/tests/drupal-7.rdf.database.php
    @@ -43,7 +43,7 @@
    diff --git a/core/phpunit.xml b/core/phpunit.xml
    

    phpunit.xml should not appear in the patch.

All the other changes such as the new line and $this->baseUri are good though. I would recommend to restart from the patch #15 and then apply your new line fixes (except the one in rdf.module and rdf.api.php and $this->baseUri.

scor’s picture

Regarding 3 and 4 of comment #21, coder review is giving error that longer than 80 cols. can you please suggest me on how I can proceed about it so that I can do all changes of comment #21 and update patch.

You and dman are right, let's leave them as you have put them in multiple lines, in other words, please ignore what I said in 3 and 4 of comment #21.

Jalandhar’s picture

Thanks scor and damn.

Regarding extra CR that I have memntioned in comment #15, the below code is working as said by damn.

'value' => "$text\n",

Updating the patch with all required changes mentioned by you.

Jalandhar’s picture

Status: Needs work » Needs review
dman’s picture

Thanks for your patience @jalandhar! Progress is being made and the end is in sight here!

Jalandhar’s picture

Hi dman and scor,

Please review the patch attached in comment #30.

TravisCarden’s picture

Status: Needs review » Needs work
FileSize
2.4 KB

This is looking really good, all. A few comments:

  1. Adding doxygen type hinting has been descoped from this initiative and moved to #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*, so those changes need to be removed from the patch. Please remember to follow steps 6 and 7 in the meta issue.
  2. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
    @@ -409,7 +418,8 @@ protected function assertRdfaArticleProperties($graph, $message_prefix) {
    -    //$this->assertEqual($graph->type($this->termUri), 'schema:Thing', 'Tag type was found (schema:Thing).');
    +    // $this->assertEqual($graph->type($this->termUri), 'schema:Thing',
    +    // 'Tag type was found (schema:Thing).');
    

    There are a number of lines of commented out code in this patch. Since the spirit of the 80 character limit for comments is to make prose readable, and since making these changes will just create extra work in the future when the lines are un-commented, let's remove these changes and treat any warnings/errors as false positives.

  3. +++ b/core/modules/rdf/tests/Drupal/rdf/Tests/RdfMappingConfigEntityUnitTest.php
    @@ -22,14 +22,16 @@ class RdfMappingEntityUnitTest extends UnitTestCase {
    -   * @var \Drupal\Core\Entity\EntityTypeInterface|\PHPUnit_Framework_MockObject_MockObject
    +   * @var \Drupal\Core\Entity\EntityTypeInterface|
    +   *   \PHPUnit_Framework_MockObject_MockObject
    

    I don't know of any precedent for splitting @var directives onto multiple lines. I feel like there's an implicit expectation of the opposite. Let's treat this as a false positive and remove these changes from the patch. Anybody want to file an issue against Coder?

  4. There are still some legitimate issues reported by Coder Sniffer with the patch applied. See sniffer.txt attached.
dman’s picture

@Jalandhar - I have to apologise that you are getting mixed messages.
We have a conflict between getting a green light from the automation vs, as @TravisCarden says, the spirit of the 80 col limit is for readability - which clearly loses here.
If it were a local project I'd shrug and leave it as a false positive, but if this task is part of a global code coverage thing, it's annoying.

I dunno.

Jalandhar’s picture

@damn: no problem...:P

@Travis carden:

I have done the changes related to all points mentioned by you in comment #34 but coder is returning errors/warnings because of changes related to 1,2 and 3. I have listed those errors in the attached file rdf_coder_errors.txt.

Please review the patch attached.

Jalandhar’s picture

Status: Needs work » Needs review

Changing status to needs review.

Jalandhar’s picture

Assigned: Jalandhar » Unassigned

Changing Assignee to Unassigned

scor’s picture

Status: Needs review » Needs work
xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Minor
Status: Needs work » Postponed
Issue tags: -Novice

Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.

tatarbj’s picture

Status: Postponed » Closed (duplicate)

Closing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.