Comments

Assigned:Unassigned» lotyrin

Status:Active» Needs review
StatusFileSize
new891 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,061 pass(es).
[ View ]

This one was easy. No doc changes.

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.

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.

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!

Issue summary:View changes
Status:Postponed» Active

Assigned:Unassigned» Jalandhar

Hi,

I have started working on it.

Status:Active» Needs review
StatusFileSize
new32.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_rdf_module_pass_coder_review-1533380-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new33.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,612 pass(es), 3 fail(s), and 10 exception(s).
[ View ]

Another try at the patch.

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.

+++ 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()

Status:Needs work» Needs review
StatusFileSize
new33.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,724 pass(es).
[ View ]

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.

Would

<?php
'value' => "$text\n",
?>

meet the requirement?

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?

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

Status:Needs work» Needs review

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.

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

<?php
'value' => "$text\n      ",
?>

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.

StatusFileSize
new40.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

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

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.

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.

  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.

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.

StatusFileSize
new33.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,849 pass(es).
[ View ]

Thanks scor and damn.

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

<?php
'value' => "$text\n",
?>

Updating the patch with all required changes mentioned by you.

Status:Needs work» Needs review

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

Hi dman and scor,

Please review the patch attached in comment #30.

Status:Needs review» Needs work
StatusFileSize
new2.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, 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.

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

StatusFileSize
new8.91 KB
new30.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,988 pass(es).
[ View ]

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

Status:Needs work» Needs review

Assigned:Jalandhar» Unassigned