Skip to content

Commit

Permalink
Get rid of event subscribers that resolve services too early, part 1
Browse files Browse the repository at this point in the history
Refs flarum/core#1578.
  • Loading branch information
franzliedke committed Dec 16, 2018
1 parent e2b0087 commit 73c0626
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 40 deletions.
7 changes: 6 additions & 1 deletion extend.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
* file that was distributed with this source code.
*/

use Flarum\Api\Event\Serializing;
use Flarum\Discussion\Event\Saving;
use Flarum\Extend;
use Flarum\Tags\Access;
use Flarum\Tags\Api\Controller;
Expand All @@ -35,11 +37,14 @@

function (Dispatcher $events) {
$events->subscribe(Listener\AddDiscussionTagsRelationship::class);

$events->subscribe(Listener\AddForumTagsRelationship::class);
$events->listen(Serializing::class, Listener\PrepareForumTagsApiAttributes::class);

$events->subscribe(Listener\CreatePostWhenTagsAreChanged::class);
$events->subscribe(Listener\FilterDiscussionListByTags::class);
$events->subscribe(Listener\FilterPostsQueryByTag::class);
$events->subscribe(Listener\SaveTagsToDatabase::class);
$events->listen(Saving::class, Listener\SaveTagsToDatabase::class);
$events->subscribe(Listener\UpdateTagMetadata::class);

$events->subscribe(Access\GlobalPolicy::class);
Expand Down
29 changes: 0 additions & 29 deletions src/Listener/AddForumTagsRelationship.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,15 @@
namespace Flarum\Tags\Listener;

use Flarum\Api\Controller\ShowForumController;
use Flarum\Api\Event\Serializing;
use Flarum\Api\Event\WillGetData;
use Flarum\Api\Event\WillSerializeData;
use Flarum\Api\Serializer\ForumSerializer;
use Flarum\Event\GetApiRelationship;
use Flarum\Settings\SettingsRepositoryInterface;
use Flarum\Tags\Tag;
use Illuminate\Contracts\Events\Dispatcher;

class AddForumTagsRelationship
{
/**
* @var SettingsRepositoryInterface
*/
protected $settings;

/**
* @param SettingsRepositoryInterface $settings
*/
public function __construct(SettingsRepositoryInterface $settings)
{
$this->settings = $settings;
}

/**
* @param Dispatcher $events
*/
Expand All @@ -44,7 +29,6 @@ public function subscribe(Dispatcher $events)
$events->listen(GetApiRelationship::class, [$this, 'getApiRelationship']);
$events->listen(WillSerializeData::class, [$this, 'loadTagsRelationship']);
$events->listen(WillGetData::class, [$this, 'includeTagsRelationship']);
$events->listen(Serializing::class, [$this, 'prepareApiAttributes']);
}

/**
Expand Down Expand Up @@ -84,17 +68,4 @@ public function includeTagsRelationship(WillGetData $event)
$event->addInclude(['tags', 'tags.lastPostedDiscussion', 'tags.parent']);
}
}

/**
* @param Serializing $event
*/
public function prepareApiAttributes(Serializing $event)
{
if ($event->isSerializer(ForumSerializer::class)) {
$event->attributes['minPrimaryTags'] = $this->settings->get('flarum-tags.min_primary_tags');
$event->attributes['maxPrimaryTags'] = $this->settings->get('flarum-tags.max_primary_tags');
$event->attributes['minSecondaryTags'] = $this->settings->get('flarum-tags.min_secondary_tags');
$event->attributes['maxSecondaryTags'] = $this->settings->get('flarum-tags.max_secondary_tags');
}
}
}
42 changes: 42 additions & 0 deletions src/Listener/PrepareForumTagsApiAttributes.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php

/*
* This file is part of Flarum.
*
* (c) Toby Zerner <toby.zerner@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Flarum\Tags\Listener;

use Flarum\Api\Event\Serializing;
use Flarum\Api\Serializer\ForumSerializer;
use Flarum\Settings\SettingsRepositoryInterface;

class PrepareForumTagsApiAttributes
{
/**
* @var SettingsRepositoryInterface
*/
protected $settings;

/**
* @param SettingsRepositoryInterface $settings
*/
public function __construct(SettingsRepositoryInterface $settings)
{
$this->settings = $settings;
}

public function handle(Serializing $event)
{
if ($event->isSerializer(ForumSerializer::class)) {
$event->attributes['minPrimaryTags'] = $this->settings->get('flarum-tags.min_primary_tags');
$event->attributes['maxPrimaryTags'] = $this->settings->get('flarum-tags.max_primary_tags');
$event->attributes['minSecondaryTags'] = $this->settings->get('flarum-tags.min_secondary_tags');
$event->attributes['maxSecondaryTags'] = $this->settings->get('flarum-tags.max_secondary_tags');
}
}
}
11 changes: 1 addition & 10 deletions src/Listener/SaveTagsToDatabase.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
use Flarum\Tags\Event\DiscussionWasTagged;
use Flarum\Tags\Tag;
use Flarum\User\Exception\PermissionDeniedException;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Contracts\Validation\Factory;
use Symfony\Component\Translation\TranslatorInterface;

Expand Down Expand Up @@ -50,20 +49,12 @@ public function __construct(SettingsRepositoryInterface $settings, Factory $vali
$this->translator = $translator;
}

/**
* @param Dispatcher $events
*/
public function subscribe(Dispatcher $events)
{
$events->listen(Saving::class, [$this, 'whenDiscussionIsSaving']);
}

/**
* @param Saving $event
* @throws PermissionDeniedException
* @throws ValidationException
*/
public function whenDiscussionIsSaving(Saving $event)
public function handle(Saving $event)
{
$discussion = $event->discussion;
$actor = $event->actor;
Expand Down

3 comments on commit 73c0626

@luceos
Copy link
Member

@luceos luceos commented on 73c0626 Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this might be an inspiration for a more definite extender for listen @franzliedke :

<?php

namespace App\Extend;

use Flarum\Extend\ExtenderInterface;
use Flarum\Extension\Extension;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Events\Dispatcher;

class Listen implements ExtenderInterface
{
    protected $events = [];

    public function extend(Container $container, Extension $extension = null)
    {
        /** @var Dispatcher $events */
        $events = $container->make(Dispatcher::class);

        foreach ($this->events as $registered) {
            $events->listen($registered[0], $registered[1]);
        }
    }

    public function listen(string $event, $listener)
    {
        $this->events[] = [
            $event,
            $listener
        ];

        return $this;
    }
}

@franzliedke
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned elsewhere, I'd like to avoid such an extender. We'll have dedicated extenders for some specific domain events (such as "post was posted" or "post was hidden"), but apart from those, events are internal, private API. Exposing this extender would just encourage using the private API.

Implementing custom extenders is still possible, but they won't be guaranteed to work across minor / patch releases, if they're using "private" API.

@luceos
Copy link
Member

@luceos luceos commented on 73c0626 Dec 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha 👍

Please sign in to comment.