Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
FileSize
30.81 KB

In CommentAttributesTest (which belongs to rdf.module, but extends CommentTestBase), there is a use of setCommentSettings() with a translated message parameter. I left that out of this patch; it should be addressed in #1798066: Clean up CommentTestBase::setCommentSettings().

Status: Needs review » Needs work

The last submitted patch, rdf-1797506-1.patch, failed testing.

xjm’s picture

Oops.

 Output: [PHP Parse error:  syntax error, unexpected ')' in ./core/modules/rdf/lib/Drupal/rdf/Tests/RdfaMarkupTest.php on line 167

I'll fix this later (or someone else is welcome to).

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
703 bytes
30.8 KB

All of the changes in #1 look good and complete to me. However, since this issue also needed to be fixed for an extra ')', I am leaving this issue to someone else to also review and approve.

Here is a patch and interdiff addressing #3.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

#4 looks good. I didn't find any more t()'s around assert messages in the RDF module.

Lars Toomre’s picture

Thanks for the review @dcam. I rolled a new patch with additions you asked for in the Node sub-issue.

webchick’s picture

Assigned: Unassigned » jhodgdon

Tum te tum...

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

This one's committed, thanks all! Ready for port...

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
28.67 KB

Backported #4 to D7.

dcam’s picture

#9: 1797506-9-t-rdf.patch queued for re-testing.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

scor’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +SprintWeekend2013

this backport is good to go.

dcam’s picture

Thanks for the review, scor!

scor’s picture

Status: Reviewed & tested by the community » Needs work
xjm’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs backport to D7, -SprintWeekend2013

#9: 1797506-9-t-rdf.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7, +SprintWeekend2013

The last submitted patch, 1797506-9-t-rdf.patch, failed testing.

xjm’s picture

Issue tags: +Needs reroll
Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
28.67 KB

Patch #9 re-rolled.

Status: Needs review » Needs work

The last submitted patch, 1797506_remove_t_assertTrue_18.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
28.76 KB

Rerolled #9.

scor’s picture

Status: Needs review » Reviewed & tested by the community

thanks @dcam. this looks good.

YesCT’s picture

Issue tags: -Needs reroll

removing reroll tag. we can add it back later if we need it.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

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