Support from Acquia helps fund testing for Drupal Acquia logo

Comments

danylevskyi’s picture

Assigned: danylevskyi » Unassigned
Status: Active » Needs review
FileSize
1.66 KB
m1r1k’s picture

m1r1k’s picture

Assigned: Unassigned » m1r1k

For easier trakcing

danylevskyi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

patch no longer applies.

m1r1k’s picture

Status: Needs work » Needs review

Push to retesting

m1r1k’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +CodeSprintCIS

The last submitted patch, forum-remove-global-user-from-forum-module-2061935-2.patch, failed testing.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
1.88 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, forum-remove-global-user-from-forum-module-2061935-9.patch, failed testing.

star-szr’s picture

Thanks for working on this @m1r1k! This needs another reroll, the patch applies with the `patch` command but not with `git apply`. Testbot uses `git apply`.

  1. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -139,7 +139,8 @@ public function getTopics($tid) {
    +    $user = $user = Drupal::currentUser();
    

    Just one $user = :)

  2. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -326,7 +327,7 @@ protected function numberNew($nid, $timestamp) {
    +    $user = Drupal::currentUser();
    

    This one and the above in the ForumManager class should be \Drupal:: since they are within a namespaced class and we don't "use Drupal". That would likely explain the test failures. See https://drupal.org/node/1353118 for more information.

m1r1k’s picture

Oh, my fault. "Haste makes waste" ... @Cottser, thank you for corrections.
Fixed

m1r1k’s picture

Status: Needs work » Needs review
star-szr’s picture

Issue tags: -Needs reroll

I'm seeing a couple other conversions are using this in OO code, but I really don't know which is the correct/preferred method.

$user = $this->container->get('current_user');
m1r1k’s picture

@Cotteser, afaik such code is using inside some OO code that have its own container, like Tests or some plugins which injects container directly.

m1r1k’s picture

Re-roll after deciding use '\Drupal' everywhere

The last submitted patch, forum-remove-global-user-from-forum-module-2061935-16.patch, failed testing.

m1r1k’s picture

Status: Needs work » Needs review
Issue tags: +CodeSprintCIS
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Applies, passes testbot and replaces all within the module. Ready to go, thanks @m1r1k!

joelpittet’s picture

Assigned: m1r1k » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
1.84 KB

Well it doesn't apply any more :S so reroll attached. Someone else to review.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -CodeSprintCIS

back to RTBC, patch still passes after re-roll.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ba69ad0 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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