Colonel_mortis

When databaseCheck is run as part of the 4.7.2 -> 4.7.3 upgrade, it ends up running additional queries to fix the schema, because they weren't included as queries by the upgrader:
ALTER TABLE `calendar_events` ADD COLUMN `event_external_url` VARCHAR (255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL , ADD COLUMN `event_latitude` DECIMAL (10,8) NULL , ADD COLUMN `event_longitude` DECIMAL (11,8) NULL ; This doesn't cause issues because it was caught by the databaseCheck, but as with the previous time that something like this happened:
this could cause problems in future.

Given the prominence of the banner, I would argue it's visual clutter for all but new members, but that does make sense. I'm just not a fan of features only showing up to non-staff, because then we don't form opinions on them (for the same reason, our admins are not exempt from ads).
I like the idea of having users choose a topic type, though I'm not sure what the solution would need to look like to be customizable enough (eg different sections have different options).

If a user is a moderator, they never see the solved_did_it_tho_title banner to prompt them to mark a post as the solution, and also don't see the prominent "mark as solution" button. This means we don't know it exists until a member complains about it.
This is because you condition both on $topic->isNotModeratorButCanSolve(), when it should really be something like $topic->isNotModeratorButCanSolve() || $topic->starter == \IPS\Member::loggedIn().
As feedback, I would like to be able to configure which forums the banner shows up in, and users get corresponding re-engagement emails - I have solutions turned on for all forums, because it is sometimes useful, but there are only a few where I truly want to push it.
 
// repost from bug report since apparently this is WAI

If a user is a moderator, they never see the solved_did_it_tho_title banner to prompt them to mark a post as the solution, and also don't see the prominent "mark as solution" button. This means we don't know it exists until a member complains about it.
This is because you condition both on $topic->isNotModeratorButCanSolve(), when it should really be something like $topic->isNotModeratorButCanSolve() || $topic->starter == \IPS\Member::loggedIn().
As feedback, I would like to be able to configure which forums the banner shows up in, and users get corresponding re-engagement emails - I have solutions turned on for all forums, because it is sometimes useful, but there are only a few where I truly want to push it.
 
// repost from bug report since apparently this is WAI

The nullability analysis doesn't take into account nulls that are explicitly in the global namespace (which is unnecessary but legal).
Hook:
public function canSetBestAnswer( \IPS\Member $member = \NULL ) Original method:
public function canSetBestAnswer( \IPS\Member $member = NULL ) Analysis:
[ "method" => "canSetBestAnswer", "reason" => "method_issue_parameters", "parameter" => null, "subclassFile" => "/applications/lmgsys/hooks/bestAnswer_ForumsTopic.php", "baseFile" => "/applications/forums/sources/Topic/Topic.php", "baseClass" => "\IPS\forums\Topic", "subclassMethod" => [ "final" => false, "security" => "public", "static" => false, "name" => "canSetBestAnswer", "parameters" => [ "member" => [ "name" => "member", "type" => "\IPS\Member", "nullable" => false, "passedByReference" => false, "packed" => false, ], ], "returnType" => null, "lineNumber" => 17, ], "subclassName" => "\IPS\forums\lmgsys_hook_bestAnswer_ForumsTopic", "baseMethod" => [ "final" => false, "security" => "public", "static" => false, "name" => "canSetBestAnswer", "parameters" => [ "member" => [ "name" => "member", "type" => "\IPS\Member", "nullable" => true, "passedByReference" => false, "packed" => false, ], ], "returnType" => null, "lineNumber" => 1388, ], "class" => "\IPS\forums\Topic", "priority" => true, ], [ "method" => "canSetBestAnswer", "reason" => "method_issue_nullable", "parameter" => "member", "subclassFile" => "/applications/lmgsys/hooks/bestAnswer_ForumsTopic.php", "baseFile" => "/applications/forums/sources/Topic/Topic.php", "baseClass" => "\IPS\forums\Topic", "subclassMethod" => [ "final" => false, "security" => "public", "static" => false, "name" => "canSetBestAnswer", "parameters" => [ "member" => [ "name" => "member", "type" => "\IPS\Member", "nullable" => false, "passedByReference" => false, "packed" => false, ], ], "returnType" => null, "lineNumber" => 17, ], "subclassName" => "\IPS\forums\lmgsys_hook_bestAnswer_ForumsTopic", "baseMethod" => [ "final" => false, "security" => "public", "static" => false, "name" => "canSetBestAnswer", "parameters" => [ "member" => [ "name" => "member", "type" => "\IPS\Member", "nullable" => true, "passedByReference" => false, "packed" => false, ], ], "returnType" => null, "lineNumber" => 1388, ], "class" => "\IPS\forums\Topic", "priority" => true, ], ],  

To reproduce:
Click https://invisioncommunity.com/profile/536475-colonel_mortis/ (note that this will not occur again if you refresh the page or use the browser back/forwards buttons - you need to actually click the link to trigger it) Observe that the page shows up for a moment, then changes to a loading spinner, before finally loading again a few seconds later. All of the page data was reloaded from the server in the process. This can be fairly jarring, especially for users on slower connections.
This is because ips.profile.main.js calls History.replaceState from setup. setup is called from initialize after the statechange event handler has already been bound. I'm not sure why you're calling History.replaceState there, but it seems like it should be safe to reorder initialize so you call setup before adding the statechange event handler.

Reported to me in https://linustechtips.com/topic/1443429-if-theres-a-username-behind-the-cursor-when-youre-reacting-to-a-post-the-hover-card-is-appended-to-the-bottom-page/ (there's a video there which illustrates the issue pretty well).
To reproduce:
Find a post that somebody else has reacted to Open the react menu, and position the cursor above the username of the member that has already reacted, eg on funny in Click to react to the post Have the request to ?do=showReactionsComment complete: After the reaction menu has closed, so that the hovercard starts getting triggered due to the hover event on the username Before the hovercard has finished loading The hovercard will appear at the bottom of the page rather than in the correct location under the cursor This happens because you take a reference to the hovered element when triggering the hovercard, but then when ?do=showReactionsComment completes the whole reactions block gets replaced, so the referenced element is no longer in the DOM.
I'm not sure what the right solution is here, though it could probably be helped by checking that the element exists in the DOM when trying to position the hovercard and aborting if not.

The symptom of this for regular status updates is that when you submit the status, it gets stuck on "Saving", despite having submitted successfully:

If you have something like
<div data-controller="foo"> <div data-controller="bar"></div> </div> ips.controller.register("foo", { initialize: function() { ips.controller.cleanContentsOf(this.scope); } }); The controller on bar will be initialized on the dead element, and never cleaned up:
ips.controller.register("bar", { initialize: function() { console.log(document.contains(this.scope[0])); // => false }, destroy: function() { console.log("destroyed"); // never called } }); This happens because ips.controller#_findControllers is called once to get all controllers to load, then the controllers are run one by one. If the foo controller is run first (and I'm not sure if that's guaranteed to be the case), cleanContentsOf will drop the bar element, but it won't run the destructor because the controller hasn't been initialized yet. When all of that logic has finished running, it then runs the bar controller, even though it is no longer applicable.
Sure, you might say, but nobody would call cleanContentsOf synchronously during init, right? Yes, you do.
Concretely, the problem this causes me is that the commentFeed of a status update exists twice, one in the document and one detatched. This means addToCommentFeed events end up getting processed by both, the detached one first, and the detached one ends up throwing an exception.
There are a few different ways this could be fixed:
Fix the double initialization of profiles. Seems straight forward enough, and that appears to be the only instance of this issue at the moment. Guard ips.controller#initControllerOnElem with node.contains(elem), to ensure that the node is still a child Run the destructor code from cleanContentsOf in a `setImmediate` block, to give the other controllers an opportunity to be initialized before getting cleaned up

Also had some members reporting today that the most recent posts were still not visible after the deletion had completed. It does look like the cached post count (used for computing the page count) was pretty far off:
mysql> SELECT posts FROM forums_topics WHERE tid=901907; +--------+ | posts | +--------+ | 143021 | +--------+ mysql> SELECT COUNT(*) FROM forums_posts WHERE topic_id=901907 AND queued=0; +----------+ | COUNT(*) | +----------+ | 143048 | +----------+ I'm not sure what caused the discrepancy to be this big, but this looks like a pretty classic race condition. Resyncing the comment counts (which would have happened organically if anyone had replied in the past few hours) has fixed it, and it's probably not straight forward to reengineer to avoid this, but it does suck.

The symptom of this for regular status updates is that when you submit the status, it gets stuck on "Saving", despite having submitted successfully:

If you have something like
<div data-controller="foo"> <div data-controller="bar"></div> </div> ips.controller.register("foo", { initialize: function() { ips.controller.cleanContentsOf(this.scope); } }); The controller on bar will be initialized on the dead element, and never cleaned up:
ips.controller.register("bar", { initialize: function() { console.log(document.contains(this.scope[0])); // => false }, destroy: function() { console.log("destroyed"); // never called } }); This happens because ips.controller#_findControllers is called once to get all controllers to load, then the controllers are run one by one. If the foo controller is run first (and I'm not sure if that's guaranteed to be the case), cleanContentsOf will drop the bar element, but it won't run the destructor because the controller hasn't been initialized yet. When all of that logic has finished running, it then runs the bar controller, even though it is no longer applicable.
Sure, you might say, but nobody would call cleanContentsOf synchronously during init, right? Yes, you do.
Concretely, the problem this causes me is that the commentFeed of a status update exists twice, one in the document and one detatched. This means addToCommentFeed events end up getting processed by both, the detached one first, and the detached one ends up throwing an exception.
There are a few different ways this could be fixed:
Fix the double initialization of profiles. Seems straight forward enough, and that appears to be the only instance of this issue at the moment. Guard ips.controller#initControllerOnElem with node.contains(elem), to ensure that the node is still a child Run the destructor code from cleanContentsOf in a `setImmediate` block, to give the other controllers an opportunity to be initialized before getting cleaned up

Noticed this locally while testing out an unrelated thing, reproed here.
Post a status update from your profile (or the create menu, or whatever) Refresh (open) your profile Status update has disappeared "See my activity" Status update isn't there, and there's no filter sidebar entry for status updates or replies I've not looked into why this is happening, other than that there is no entry in the system log (while not IN_DEV), and rebuilding the search index didn't resolve it (though it was also empty after rebuilding, so YMMV).
I no longer use (stock IPS) status updates on my site, and the impression I got from the lack of support previously was that we were the primary user of them, so maybe nobody will notice?

If you have something like
<div data-controller="foo"> <div data-controller="bar"></div> </div> ips.controller.register("foo", { initialize: function() { ips.controller.cleanContentsOf(this.scope); } }); The controller on bar will be initialized on the dead element, and never cleaned up:
ips.controller.register("bar", { initialize: function() { console.log(document.contains(this.scope[0])); // => false }, destroy: function() { console.log("destroyed"); // never called } }); This happens because ips.controller#_findControllers is called once to get all controllers to load, then the controllers are run one by one. If the foo controller is run first (and I'm not sure if that's guaranteed to be the case), cleanContentsOf will drop the bar element, but it won't run the destructor because the controller hasn't been initialized yet. When all of that logic has finished running, it then runs the bar controller, even though it is no longer applicable.
Sure, you might say, but nobody would call cleanContentsOf synchronously during init, right? Yes, you do.
Concretely, the problem this causes me is that the commentFeed of a status update exists twice, one in the document and one detatched. This means addToCommentFeed events end up getting processed by both, the detached one first, and the detached one ends up throwing an exception.
There are a few different ways this could be fixed:
Fix the double initialization of profiles. Seems straight forward enough, and that appears to be the only instance of this issue at the moment. Guard ips.controller#initControllerOnElem with node.contains(elem), to ensure that the node is still a child Run the destructor code from cleanContentsOf in a `setImmediate` block, to give the other controllers an opportunity to be initialized before getting cleaned up

I'm a bit late, but I have some updated numbers based on the past 3 months of registrations. In the latest data:
Precision (proportion of users with a spam score >1 that are actually spammers) is 81%, which is up significantly on the 51% from 2020 Recall (proportion of spammers that get a spam score >1) is 24%, which is down slightly (but probably within margin of error) on the 28% from 2020 However, the number of spammers relative to actual users has increased since the data in the original post, which means the precision is not comparable. Correcting for that changes the precision to 66%, which is still a decent improvement over 2020.
The biggest change for us though is that the spam service can mod queue members rather than blocking their registration outright, which means the sub-optimal precision is not as important as it was. This, combined with a collection of banned words, banned links, and custom spam rules (via a custom plugin), has meant that over 90% of the spam posts were mod queued before being posted.

solved this one awhile back 
 

I'm a bit late, but I have some updated numbers based on the past 3 months of registrations. In the latest data:
Precision (proportion of users with a spam score >1 that are actually spammers) is 81%, which is up significantly on the 51% from 2020 Recall (proportion of spammers that get a spam score >1) is 24%, which is down slightly (but probably within margin of error) on the 28% from 2020 However, the number of spammers relative to actual users has increased since the data in the original post, which means the precision is not comparable. Correcting for that changes the precision to 66%, which is still a decent improvement over 2020.
The biggest change for us though is that the spam service can mod queue members rather than blocking their registration outright, which means the sub-optimal precision is not as important as it was. This, combined with a collection of banned words, banned links, and custom spam rules (via a custom plugin), has meant that over 90% of the spam posts were mod queued before being posted.

I'm a bit late, but I have some updated numbers based on the past 3 months of registrations. In the latest data:
Precision (proportion of users with a spam score >1 that are actually spammers) is 81%, which is up significantly on the 51% from 2020 Recall (proportion of spammers that get a spam score >1) is 24%, which is down slightly (but probably within margin of error) on the 28% from 2020 However, the number of spammers relative to actual users has increased since the data in the original post, which means the precision is not comparable. Correcting for that changes the precision to 66%, which is still a decent improvement over 2020.
The biggest change for us though is that the spam service can mod queue members rather than blocking their registration outright, which means the sub-optimal precision is not as important as it was. This, combined with a collection of banned words, banned links, and custom spam rules (via a custom plugin), has meant that over 90% of the spam posts were mod queued before being posted.

Of the 147 members who were classified as 4 but not FASed, about 30 are likely spam accounts (based on manual classification by one of my moderators), which gives a precision of 61% (39% false positive rate) when classifying based on a score of 4.
I'd be happy to share some more detailed data with you if there's anything that you think would be helpful - feel free to reach out by ticket/email/PM(/slack), whatever is easiest.

To reproduce:
Open "mark as solution" in a new tab several times Look at the author's solutions list, and observe that the same post shows up multiple times, and the user's solution count has increased accordingly. This can be triggered accidentally, eg if the user accidentally clicks the button multiple times or has a network dropout, and could also be exploited by users to quietly artificially boost their solution count.
It happens because in \IPS\Content\Solvable::toggleSolveComment, you don't delete the existing entry from core_solved_index when marking a new post as a solution (though you do clear the solved flag from the existing post). You also don't remove the duplicate notifications.
(This could also be solved with a unique index on core_solved_index on comment_class+item_id.)

This means the new min version will be higher than the old max version, right? That's really unfortunate for orchestrating the upgrade.

This means the new min version will be higher than the old max version, right? That's really unfortunate for orchestrating the upgrade.

One frequent complaint that I see is that every question is mandatory, which forces members to add a "not applicable" option when they have multiple questions. The poll title is a waste of time, given that there's already a topic title and poll question title I think it would be really neat if polls were attached to posts rather than to topics - that would force people to actually read the first post before answering the poll, and would allow for people to add polls in replies (and other places such as status updates) too

If there are polls in the middle of the topic then yeah they could definitely get lost, but I would imagine polls like that to be pretty ephemeral anyway - if you want a poll that everyone sees, you add it to the first post (or recommend it).

One frequent complaint that I see is that every question is mandatory, which forces members to add a "not applicable" option when they have multiple questions. The poll title is a waste of time, given that there's already a topic title and poll question title I think it would be really neat if polls were attached to posts rather than to topics - that would force people to actually read the first post before answering the poll, and would allow for people to add polls in replies (and other places such as status updates) too