Skip to content

Commit

Permalink
Fix best answer tag behavior causing errors & unintended side-effects
Browse files Browse the repository at this point in the history
It could delete all tags if the array was empty (due to calling `$tags->detatch([])`).

It could also crash the forum if the setting value wasn't a valid JSON string (eg. `null`)
  • Loading branch information
dsevillamartin committed Sep 16, 2023
1 parent 326f479 commit bdf4b5b
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 3 deletions.
4 changes: 3 additions & 1 deletion extend.php
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@
->serializeToForum('useAlternativeBestAnswerUi', 'fof-best-answer.use_alternative_ui', 'boolVal')
->serializeToForum('showBestAnswerFilterUi', 'fof-best-answer.show_filter_dropdown', 'boolVal')
->serializeToForum('fof-best-answer.show_max_lines', 'fof-best-answer.show_max_lines', 'intVal')
->serializeToForum('fof-best-answer.tags', 'fof-best-answer.select_best_answer_tags', 'json_decode')
->serializeToForum('fof-best-answer.tags', 'fof-best-answer.select_best_answer_tags', function ($val) {
return @json_decode($val, false) ?? [];
})
->default('fof-best-answer.schedule_on_one_server', false)
->default('fof-best-answer.stop_overnight', false)
->default('fof-best-answer.store_log_output', false),
Expand Down
24 changes: 22 additions & 2 deletions src/Listeners/SaveBestAnswerToDatabase.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Flarum\Notification\Notification;
use Flarum\Notification\NotificationSyncer;
use Flarum\Post\Post;
use Flarum\Tags\Tag;
use Flarum\User\Exception\PermissionDeniedException;
use Flarum\User\User;
use FoF\BestAnswer\BestAnswerRepository;
Expand Down Expand Up @@ -94,6 +95,7 @@ private function removeBestAnswer(Discussion $discussion, User $actor): void
$discussion->best_answer_post_id = null;
$discussion->best_answer_user_id = null;
$discussion->best_answer_set_at = null;
$discussion->unsetRelation('bestAnswerPost');

$this->changeTags($discussion, 'detach');

Expand Down Expand Up @@ -137,8 +139,26 @@ private function setBestAnswer(Discussion $discussion, User $actor, int $id): vo

protected function changeTags(Discussion $discussion, string $method)
{
$tagsToChange = json_decode(resolve('flarum.settings')->get('fof-best-answer.select_best_answer_tags'));
$tagsToChange = @json_decode(resolve('flarum.settings')->get('fof-best-answer.select_best_answer_tags'));

$discussion->tags()->$method($tagsToChange);
if (empty($tagsToChange)) {
return;
}

$validTags = Tag::query()->whereIn('id', $tagsToChange);

// Query errors if we try to attach tags that are already attached due to the unique constraint
if ($method === 'attach') {
$existingTags = $discussion->tags()->pluck('id');
$validTags = $validTags->whereNotIn('id', $existingTags);
}

$validTagsIds = $validTags->pluck('id');

if ($validTagsIds->isEmpty()) {
return;
}

$discussion->tags()->$method($validTagsIds);
}
}

0 comments on commit bdf4b5b

Please sign in to comment.