[2.x] chore(flags): dispatch model events when flags are deleted

[DavideIadeluca]

Fixes #0000
No eloquent model events being dispatched when a flag gets deleted

Changes proposed in this pull request:

• Change how flags are deleted. With the new approach model events are fired after each delete, allowing Observers to hook into this.
• This in turn also allows the Audit Integration to loose some complexity with the macro

Reviewers should focus on:

Screenshot

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

Required changes:

• Related documentation PR: (Remove if irrelevant)

[imorland]

The direction here is good — replacing the global HasMany::macro('delete') with an explicit Dismissed event is a real improvement and takes some fragile complexity out of AuditIntegration. A few things need addressing before it can go in, though.

Flags are deleted in two places, and only one is covered. DeleteFlagsHandler dismisses via the UI, but Listener/DeleteFlags also deletes a post's flags in response to Post\Event\Deleted (when a flagged post is deleted). The old macro hooked HasMany::delete globally, so it caught both paths; the new Dismissed event is only dispatched from the command handler. As it stands, deleting a flagged post no longer logs post.dismissed_flags — that's a regression from the current behaviour.

The stated goal isn't fully met for the same reason. The point of the change is to fire model events on flag deletion so observers can hook in, but Listener/DeleteFlags still uses a bulk ->delete(), which fires no model events. So an observer would see UI dismissals but silently miss the post-deletion case.

Needs test coverage for the post-deletion path. AuditTest::delete() only exercises the command path (which still works), so this gap passes CI unnoticed. Please add a test that deletes a flagged post and asserts the dismissal is handled (audit log entry / events fired), so the second path is actually covered.

So: apply the same treatment to Listener/DeleteFlags so both deletion paths behave consistently, and add the post-deletion test. Happy to look again once that's in.

[DavideIadeluca]

@imorland Thanks for the review — I dug into the post-deletion path and the regression turns out not to exist.

flags.post_id has had ON DELETE CASCADE since 2018. Post\Event\Deleted fires after the row is deleted, so the old listener's flags()->delete() always hit an already-empty relation — the macro's count() guard was always 0 and post.dismissed_flags was never logged on post deletion. The macro only ever fired on the command path, which still works.

That's also why the macro was semantically questionable: it hooked HasMany::delete globally and inferred "flags were dismissed" from any bulk delete on a post's flags relation. But deleting a flagged post isn't a dismissal — the flag was acted upon, not dismissed. The frontend already reflects this: the destructive control on a flagged post also calls the flags.delete endpoint, so all actual moderator dismissals go through DeleteFlagsHandler and dispatch Dismissed with a real actor. So Dismissed intentionally stays command-path-only.

For model events: hooking Deleted can never fire them (the cascade already removed the rows), so I moved Listener/DeleteFlags to Post\Event\Deleting and delete each flag via Eloquent while they still exist. Observers now see both paths — which the old code never achieved.

One trade-off with listening to Deleting: the flags are removed before the post delete is final, so if the deletion were to abort after the event, the flags would already be gone. Previously the cascade removed them atomically with the post row. The window is a single delete statement on the API path, so I think it's acceptable — but if we want to avoid it entirely, we could snapshot the flags on Deleting (loadMissing('flags')) and run their Eloquent deletes on Deleted: the rows are already cascade-deleted by then, but Model::delete() still fires the model events. Happy to go that route if you prefer.

Post-deletion path is now covered: DeleteFlagsOnPostDeletionTest asserts model events fire per flag, and AuditTest::deletingFlaggedPostDoesntLogFlagDismissal pins down that no audit entry is written (matching pre-existing behaviour).

[imorland]

Thanks Dave — really appreciate you digging into the cascade timing, and the "acted upon vs dismissed" framing is a genuinely good point that I want to come back to. But before that, I went and tested the post-deletion path directly, and it turns out the old code does log post.dismissed_flags when a flagged post is deleted — so there is a behaviour change here, just not the one either of us assumed.

I think the cascade reasoning has one gap: at Post\Event\Deleted time the flag rows are still visible to the same connection, so the old listener's flags()->delete() isn't hitting an empty relation — the macro's count() is non-zero and it logs. Rather than argue it, here's a small harness so you can see it on your own machine. Same file, run on each branch:

extensions/flags/tests/integration/PostDeletionDismissalProofTest.php

<?php

namespace Flarum\Flags\Tests\integration;

use Carbon\Carbon;
use Flarum\Audit\AuditLog;
use Flarum\Audit\Tests\integration\InteractsWithAuditLog;
use Flarum\Discussion\Discussion;
use Flarum\Flags\Flag;
use Flarum\Post\Post;
use Flarum\Testing\integration\TestCase;
use PHPUnit\Framework\Attributes\Test;

class PostDeletionDismissalProofTest extends TestCase
{
    use InteractsWithAuditLog;

    public function setUp(): void
    {
        parent::setUp();

        $this->setUpAuditLog();
        $this->extension('flarum-audit', 'flarum-flags');

        $date = Carbon::parse('2021-01-01T12:00:00+00:00');

        // Flags are seeded directly (not via the flag API), so setUp logs nothing.
        $this->prepareDatabase([
            Discussion::class => [
                ['id' => 10, 'title' => 'A', 'created_at' => $date, 'last_posted_at' => $date, 'first_post_id' => 1, 'last_post_id' => 2, 'last_post_number' => 2, 'comment_count' => 2],
            ],
            Post::class => [
                ['id' => 1, 'number' => 1, 'discussion_id' => 10, 'created_at' => $date, 'type' => 'comment', 'content' => '<t><p>A</p></t>'],
                ['id' => 2, 'number' => 2, 'discussion_id' => 10, 'created_at' => $date, 'type' => 'comment', 'content' => '<t><p>B</p></t>'],
            ],
            Flag::class => [
                ['id' => 20, 'post_id' => 2],
                ['id' => 21, 'post_id' => 2],
            ],
        ]);
    }

    /** Control: setUp creates no dismissal entry, so anything found below is from the DELETE. Passes on both branches. */
    #[Test]
    public function control_no_dismissal_entry_exists_before_any_delete(): void
    {
        $this->app();

        $this->assertSame(0, AuditLog::query()->where('action', 'post.dismissed_flags')->count());
        $this->assertSame(2, Flag::query()->where('post_id', 2)->count());
    }

    /** Delete the flagged post (not its flags), then check for a dismissal log. 2.x: PASSES. PR: FAILS. */
    #[Test]
    public function logsDismissal_onPostDeletion(): void
    {
        $this->sendSuccessfulRequest('DELETE', '/api/posts/2', [], 204);

        $this->assertNull(Post::query()->find(2));
        $this->assertSame(0, Flag::query()->where('post_id', 2)->count());

        $this->assertLogExists('post.dismissed_flags', [
            'discussion_id' => 10,
            'post_id' => 2,
        ]);
    }

    /** Same event as a raw count in the message. 2.x -> 1, PR -> 0. */
    #[Test]
    public function report_dismissal_count_after_post_deletion(): void
    {
        $this->sendSuccessfulRequest('DELETE', '/api/posts/2', [], 204);

        $count = AuditLog::query()->where('action', 'post.dismissed_flags')->count();

        $this->assertSame(0, $count, "post.dismissed_flags entries after DELETE /api/posts/2 = {$count} (2.x=1, PR=0)");
    }
}

Running the same file on each branch:

	control	logsDismissal_onPostDeletion	report (count)
2.x	pass	pass — entry written	fail, prints 1
this PR	pass	fail — no entry	pass, count 0

control passing on both sides rules out a setup artefact, so the flip is purely the post-deletion path. So the PR does remove the dismissal log on that path — that's the behaviour change I flagged originally.

Now, the more interesting bit: I actually think your instinct might be right — deleting a flagged post arguably isn't a "dismissal", and it's worth questioning whether the old log was ever the correct thing to record. But that makes this a deliberate change to audit semantics rather than a no-op, and I'd want us to decide it on purpose:

• If we agree the post-deletion path shouldn't log a dismissal, let's say so explicitly and keep a test that pins the new behaviour (no entry) as intended — and it's worth a thought as to whether anything else should be recorded when a moderator makes flagged content disappear, so the audit trail doesn't just go quiet.
• If we'd rather preserve the existing audit trail, the listener needs to log on the post-deletion path too.

Either's fine by me — I just don't want it changing silently. What's your read on whether that post-deletion log is worth keeping? The model-events rework itself (moving to Deleting, dropping the HasMany macro) is a solid improvement regardless of which way we go.

[DavideIadeluca]

Thanks Dave — really appreciate you digging into the cascade timing, and the "acted upon vs dismissed" framing is a genuinely good point that I want to come back to. But before that, I went and tested the post-deletion path directly, and it turns out the old code does log post.dismissed_flags when a flagged post is deleted — so there is a behaviour change here, just not the one either of us assumed.
..
Now, the more interesting bit: I actually think your instinct might be right — deleting a flagged post arguably isn't a "dismissal", and it's worth questioning whether the old log was ever the correct thing to record. But that makes this a deliberate change to audit semantics rather than a no-op, and I'd want us to decide it on purpose:

If we agree the post-deletion path shouldn't log a dismissal, let's say so explicitly and keep a test that pins the new behaviour (no entry) as intended — and it's worth a thought as to whether anything else should be recorded when a moderator makes flagged content disappear, so the audit trail doesn't just go quiet.
If we'd rather preserve the existing audit trail, the listener needs to log on the post-deletion path too.
Either's fine by me — I just don't want it changing silently. What's your read on whether that post-deletion log is worth keeping? The model-events rework itself (moving to Deleting, dropping the HasMany macro) is a solid improvement regardless of which way we go.

Yes It's a behaviour change when compared to the default branch right now, however this change was only introduced with the 2.x rewrite of kilowhat/flarum-ext-audit-pro in #4711 just 6 weeks ago in RC4. So I wouldn't put that much weight on the behaviour change and with that recommend that the post-deletion path shouldn't log a post dismissed event as was the case in the 1.x distribution of Audit Pro. The added test already asserts this.

I think the cascade reasoning has one gap: at Post\Event\Deleted time the flag rows are still visible to the same connection, so the old listener's flags()->delete() isn't hitting an empty relation — the macro's count() is non-zero and it logs. Rather than argue it, here's a small harness so you can see it on your own machine. Same file, run on each branch:

This is interesting, I'm observing the complete opposite on my local machine. There must be some environment differences or I understand you incorrectly Can you try running the following SQL on your local machine and share the output (prerequisit being that your local dev has at least one flag)?

SET @p = (SELECT post_id FROM flags LIMIT 1);
SELECT @p AS probe_post_id;
START TRANSACTION;
SELECT COUNT(*) AS before_delete FROM flags WHERE post_id = @p;
DELETE FROM posts WHERE id = @p;
SELECT COUNT(*) AS after_delete FROM flags WHERE post_id = @p;
ROLLBACK;
SELECT COUNT(*) AS after_rollback FROM flags WHERE post_id = @p;

This is what I'm getting:

probe_post_id 228
before_delete 1
after_delete 0
after_rollback 1

if I understand you correctly on your machine after_delete is 1?

Output of my php flarum info

root@f37cd590c5c0:/var/www# php flarum info
Flarum core: 2.0.0-rc.5
PHP version: CLI: 8.3.32, Web: 8.3.32
PHP memory limit: CLI: 512M, Web: 512M
MySQL version: 8.0.44
Loaded extensions: Core, date, libxml, openssl, pcre, sqlite3, zlib, ctype, curl, dom, fileinfo, filter, hash, iconv, json, mbstring, SPL, session, PDO, pdo_sqlite, standard, posix, random, readline, Reflection, Phar, SimpleXML, tokenizer, xml, xmlreader, xmlwriter, mysqlnd, calendar, exif, gd, intl, pcntl, pdo_mysql, redis, sockets, sodium, zip, Zend OPcache, xdebug
+----------------------+--------------+------------------------------------------+-------+
| Flarum Extensions    |              |                                          |       |
+----------------------+--------------+------------------------------------------+-------+
| ID                   | Version      | Commit                                   | Notes |
+----------------------+--------------+------------------------------------------+-------+
| flarum-audit         | 2.x-dev      |                                          |       |
| flarum-tags          | 2.x-dev      |                                          |       |
| flarum-flags         | 2.x-dev      |                                          |       |
| flarum-sticky        | 2.x-dev      |                                          |       |
| flarum-lock          | 2.x-dev      |                                          |       |
| flarum-approval      | 2.x-dev      |                                          |       |
| fof-clockwork        | 2.0.0-beta.2 |                                          |       |
| fof-blog             | 1.x-dev      | b93e9df2fe23b959e227d0444d6c8dc1a23b37f4 |       |
| flarum-suspend       | 2.x-dev      |                                          |       |
| flarum-subscriptions | 2.x-dev      |                                          |       |
| flarum-statistics    | 2.x-dev      |                                          |       |
| flarum-nicknames     | 2.x-dev      |                                          |       |
| flarum-messages      | 2.x-dev      |                                          |       |
| flarum-mentions      | 2.x-dev      |                                          |       |
| flarum-markdown      | 2.x-dev      |                                          |       |
| flarum-likes         | 2.x-dev      |                                          |       |
| flarum-lang-spanish  | 2.x-dev      |                                          |       |
| flarum-lang-italian  | 2.0.1        |                                          |       |
| flarum-lang-german   | 2.0.10       |                                          |       |
| flarum-lang-french   | v5.0.6       |                                          |       |
| flarum-lang-english  | 2.x-dev      |                                          |       |
| flarum-gdpr          | 2.x-dev      |                                          |       |
| flarum-emoji         | 2.x-dev      |                                          |       |
| flarum-bbcode        | 2.x-dev      |                                          |       |
+----------------------+--------------+------------------------------------------+-------+
Base URL: http://localhost
Installation path: /var/www
Queue driver: sync
Session driver: file
Scheduler status: Never run
Mail driver: log
Debug mode: ON

Don't forget to turn off debug mode! It should never be turned on in a production system.