Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Samvel’s picture

Assigned: Unassigned » Samvel
Samvel’s picture

Status: Active » Needs review
FileSize
4.06 KB

attached

Samvel’s picture

Issue tags: +CodeSprintUA

+ CodeSprintUA

thedavidmeister’s picture

Status: Needs review » Needs work

A few of the renderable arrays go past 80 characters. Could they be split over multiple lines please?

Samvel’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

I found 1 critical, 2 normal and 1 low places.
Critical and normal fixed.

Attached new one.

thedavidmeister’s picture

Status: Needs review » Needs work
+    $build = array('#theme' => 'forum_submitted', '#topic' => $forum->last_post);
+      $created = array('#theme' => 'forum_submitted', '#topic' => $topic);
+    $build = array('#theme' => 'username', '#account' => $variables['topic']);
+    $breadcrumb_build = array(
+    $breadcrumb_build = array(

These variables should all be the name of the #theme, eg. use $forum_submitted instead of $build. The only time you don't want to do this is when it would override the value of another variable already in the current scope.

See the issue summary of the parent issue for details.

adamcowboy’s picture

Status: Needs work » Needs review

I tried to make the changes.

adamcowboy’s picture

FileSize
4.15 KB

Status: Needs review » Needs work

The last submitted patch, twig-2009574-7.patch, failed testing.

adamcowboy’s picture

Status: Needs work » Needs review
FileSize
4.14 KB
2.23 KB

I tried again :D. Interdiff included

Status: Needs review » Needs work

The last submitted patch, twig-2009574-9.patch, failed testing.

adamcowboy’s picture

Status: Needs work » Needs review
FileSize
4.15 KB

Go Go Bot!

Status: Needs review » Needs work

The last submitted patch, twig-2009574-12.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
Issue tags: -CodeSprintUA
FileSize
4.43 KB

Rerolled. :)

e2tha-e’s picture

Assigned: Samvel » e2tha-e

I'm working on this

e2tha-e’s picture

FileSize
3.85 KB

Rererolled :))

pplantinga’s picture

Status: Needs review » Needs work

Surely we can come up with a better variable name than $crubms?

e2tha-e’s picture

Status: Needs work » Needs review
FileSize
3.87 KB

The name of array $crubms has been changed to $breadcrumb. The $breadcrumb array is rolled into a render array named $breadcrumb_build as per the precedent set by the verifyForumView function in the same class ForumTest.

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Cool. Looks good to me!

e2tha-e’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.21 KB

Switched the names of the $breadcrumb and $breadcrumb_build arrays in functions verifyForums and verifyForumView function in class ForumTest. This is to abide by the naming convention requested in comment #6.

thedavidmeister’s picture

Status: Needs review » Needs work

+ $breadcrumb= array(

Missing space after $breadcrumb.

After that, seems fine.

e2tha-e’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

Spacing fixed. Rerolled.

Status: Needs review » Needs work

The last submitted patch, twig-forum-2009574-22.patch, failed testing.

e2tha-e’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

Patch rerolled after pulling latest from git repo.

siccababes’s picture

Status: Needs review » Reviewed & tested by the community

I tested this patch by creating a forum topic and commenting. I also added the two blocks relevant to the forum. Everything looks good.

pplantinga’s picture

Status: Reviewed & tested by the community » Needs work

What happened to changes in ForumTest.php? They seem to be missing from latest patch.

e2tha-e’s picture

core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php has been removed from the latest git revisions. I don't know anything more than that.

pplantinga’s picture

Actually I don't think it has:

https://api.drupal.org/api/drupal/core!modules!forum!lib!Drupal!forum!Te...

Check again. Have you accidentally checked in a change that removes that file? Do a clean git reset --hard 8.x or git clone

e2tha-e’s picture

Status: Needs work » Needs review
FileSize
4.22 KB

Rerolled with correctly pulled git. Changes to ForumTest.php restored.

siccababes’s picture

Status: Needs review » Reviewed & tested by the community

I tested this patch by creating a forum topic and commenting on it. I also added the two blocks pertaining to the forum. Everything looked good.

xjm’s picture

Issue tags: -Novice

#29: twig-forum-2009574-29.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, twig-forum-2009574-29.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
Issue tags: +Novice

#29: twig-forum-2009574-29.patch queued for re-testing.

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and passes manual testing (#30)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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