As designed Deleting thread by deleting first post bypasses \XF\Service\Thread\Deleter

[Jake B.]

Affected version

2.X

Seems that if you delete the first post in a thread it just directly runs $thread->softDelete or $thread->delete rather than using the thread deleter service.

 

[Chris D]

I can understand where you're coming from with this, but generally it's not essential to use a service to perform a specific task, especially if the additional functionality of that service isn't required (moderator alerts in this case).

I assume you are hooking into the Deleter service and whatever you're performing there is being missed in this case?

Generally if you want to do something absolutely every time a task is performed, the only reliable way to do that is to hook into the entity life cycle stuff. Specifically this is either extending softDelete/delete or _postSave / _postDelete in this case.

If I'm missing some other obvious use case, please let us know, but I'll call this As designed for now.

 

[Jake B.]

The main issue is that \XF\Service\Thread\Deleter has a setUser function that will allow you to set the user that is performing the delete. AFAIK this doesn't make a difference in anything by default since everything is deleted by visitor, but I could imagine cases where an add-on would set a different delete user than visitor (such as a crowd moderation add-on that would automatically soft delete a post after X reports). In my specific use case I need to get the user that actually deleted the thread

 

[Chris D]

Actually, if anything, that is the bug. We slightly changed our outlook on setting users inside services during development of XF 2.0. We tend to not allow it. You'll notice the majority of the setUser methods are actually protected and only set internally, and this is by design.

The general reasoning for that is that it would cause unexpected behaviour in some cases if the user was changed part way through the life of the service. As well as not really making a lot of sense, there would be potential to introduce inconsistent data in some cases.

The preferred approach in most cases, and that applies here to the Post Deleter, is to perform the action within a \XF::asVisitor() closure.

 

[Jake B.]

Actually, if anything, that is the bug. We slightly changed our outlook on setting users inside services during development of XF 2.0. We tend to not allow it. You'll notice the majority of the setUser methods are actually protected and only set internally, and this is by design.

Should I make a separate bug report for that? That's the only reason I had needed this since I assume people are going to use that method since it's available

The preferred approach in most cases, and that applies here to the Post Deleter, is to perform the action within a \XF::asVisitor() closure.

If that is what you recommend for people to do (and people actually do it that way) then what I'm doing now should work fine

 

[Chris D]

Should I make a separate bug report for that?

I'm not sure we could feasibly fix it at this point. Changing method visibility would be a backwards compatibility break.

We'd just have to rely on people recognising that using the method like that may not work as expected.