Jump to content

Colonel_mortis

Clients
  • Posts

    1,452
  • Joined

  • Last visited

  • Days Won

    5

 Content Type 

Downloads

Release Notes

IPS4 Guides

IPS4 Developer Documentation

Invision Community Blog

Development Blog

Deprecation Tracker

Providers Directory

Forums

Events

Store

Gallery

Everything posted by Colonel_mortis

  1. If I have a manually added quote (ie a quote added using the quote button rather than by quoting another post), it is often a relevant part of the actual post (especially when it's in the first post in a topic), so it would make sense for those quotes to remain as nested quotes when that post is quoted (especially now long quotes are truncated). It wouldn't be too hard to achieve - just look for the attribution attributes on the quote before stripping it.
  2. I realise this ship has probably long since sailed, but we're 3 months into 4.5.4, and there's a decent pile of bugs, including a currently unpatched security related one, that have piled up in that time. Is there any chance we can have a 4.5.5 release to fix some of those bugs, rather than watching 4.6 stretch further and further out? I pushed for the features to be included in 4.6 rather than 4.5.5, but I did not intend for that to be at the expense of the timely release of other changes. (I also recognise wanting to give staff a break etc, but we're well into 2021 now, and surely having more bugs fixed would reduce their load anyway?)
  3. Another one I've just run into - \IPS\Content\Comment::contentImages calls $item->container() indiscriminately, which results in a bad method call exception. This can be triggered when viewing an activity stream RSS feed.
  4. When an ips.ui.editor instance is destroyed, you are following the answer from a Stack Overflow question and trying to manually destroy the CKE instance. Unfortunately, that manual destruction is not sufficient, and is leaving references to the editor and behaviour. Take ipsautosave as an example (and maybe the only example, I've not looked further) - it maintains a reference to `editor`, and runs autosave on it every two seconds. It attaches a `destroy` event handler, which would be triggered when calling editor.destroy(), to cancel the timeout and therefore release the editor reference, but because you never trigger the destroy event it's not sufficient. (I'm noticing this because I am trying to use multiple editors on a page with the same autosave key, but it's still a problem without that.) It looks like there are a few other references that destroy manually cleans up, so there are presumably a few other things that are leaked too. At a minimum, you should be doing `editor.fire("destroy")` to trigger all of that cleanup code, but ideally you should find the things that prevent editor.destroy() from working and fix those in CKE. As a middle ground, maybe you could do `try { editor.destroy(); } catch (e) { /* existing manual best effort */ }`.
  5. Ugh it's the patch from ticket #70349 again. Looking at the patch again it doesn't seem to break either of the things that I've run into, so it seems I somehow managed to misapply it (my linux git doesn't like that patch file at all, it's missing the a/ and b/ prefixes). Apologies for wasting your time twice for that.
  6. If you've not discovered it yet, I'm also getting TypeError: array_merge(): Argument #2 must be of type array, null given (0) in \IPS\forums\Forum::formatFormValues:1120 (neither $whereString nor $whereParams are defined as far as I and my IDE can tell, and $where seems to be unused). Also there are a few places where you have new\DateInterval( which seems to now be lexed as calling the function DateInterval in the namespace new, rather than applying the new operator to the root-namespace class DateInterval [1].
  7. I've only been playing around with it IN_DEV so far, but I'm not noticing a difference. I don't think the JIT compiler is expected to make too much difference for web apps, but I would love to be wrong there.
  8. PHP 8.0.0 has been released. Some bugs that I've found in a couple of minutes of testing: There are a bunch of deprecation warnings for required parameters following optional parameters - from my search with the regex function\s*\w+\s*\([^=)]*=[^)]*,\s*\$[^,)=]*[,)] there are 47 instances of this in the suite. It should be safe to remove all of the offending default parameters, since they can never be utilised. The cms lang key can_edit_item_message_record is invalid - it contains %S rather than %s, which causes the page to (randomly?) 500. Fatal error: Cannot make static method XMLReader::open() non static in class IPS\Xml\_XMLReader in .../system/Xml/XMLReader.php on line 34 (this might be a fun one to fix - it's a PHP 8 change that isn't compatible in either direction - https://www.php.net/manual/en/xmlreader.open.php#refsect1-xmlreader.open-changelog - you probably have to make a new method that delegates instead) TypeError: method_exists(): Argument #1 ($object_or_class) must be of type object|string, null given from statusContainer.phtml:133 (when viewing a feed containing status updates) PHP Fatal error: Unparenthesized `a ? b : c ? d : e` is not supported. Use either `(a ? b : c) ? d : e` or `a ? b : (c ? d : e)` in .../applications/calendar/widgets/upcomingEvents.php on line 153 If I come across any more issues, I'll add them in this topic, and encourage others to do the same.
  9. It's also redundant when referring to a post within a topic - the title is displayed twice, with different formatting each time.
  10. 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.
  11. Actually, looking further into the members who were caught by the spam service but weren't flagged as spammers, there are several who have made 0 posts (and thus weren't caught in my previous audit) but who are likely to be actual spammers based on their profile information. Based on the sample that I checked, the false positive rate is still too high to be useful, but it is not as high as I had originally thought.
  12. I run a large site, so I have a lot of legitimate members and receive a fairly significant number of spammers. I turned it off (but continued to collect members' spam scores) several months ago, due to the volume of requests by legitimate people who were being caught by the spam filter. I've run some statistics based on registrations over the past month, comparing whether they have been flagged as spammer (which is very closely correlated with whether they were actually a spammer (this may not be true - see my post later in the topic)) and the code that the IPS spam service gave. The results are: +-------+------+----------+ | FASed | code | count | +-------+------+----------+ | 0 | 0 | 77 | | 0 | 1 | 5657 | | 0 | 2 | 3 | | 0 | 3 | 5 | | 0 | 4 | 147 | | 0 | null | 233 | | 1 | 0 | 3 | | 1 | 1 | 616 | | 1 | 2 | 13 | | 1 | 3 | 75 | | 1 | 4 | 155 | | 1 | null | 2 | +-------+------+----------+ Breaking this down: 12% of registrations are spammers 6% of registrations receive a spam score > 1 4% of registrations receive a spam score of 4 So far, these numbers don't seem unreasonable. However, If someone receives a spam score of 4 ("user is a known spammer"), they have 51% chance of actually being a spammer A precision of 51% is totally useless. If someone is actually a spammer, they have a 28% chance of receiving a spam score > 1. That's a pretty shoddy recall too. If I set my site to reject members with a spam score of 4, I will lose ~150 members to the spam filter each month; even if I use 2 as the threshold instead it will still only reduce spam by 28%. That's not an OK trade-off to me. You may say that the answer to this is to set accounts to require admin validation instead when caught by the spam filter, and that's not unreasonable. However, I'm not logged into ACP every day, so I suspect this would result in the loss of a large portion of the potential registrations who will just go and ask their question on a different forum because they didn't want to wait. Furthermore, it's often not possible to tell whether the account is legitimate just based on the registration info, so that 28% hit rate is going to drop quite a lot more. I appreciate that catching spam is a very hard problem. However, I believe these number demonstrate that the current system is not fit for purpose, at least with the level of confidence that you currently assign to it ("member is a known spammer" in the config page is a long way from the truth, and "certain spammer" from your marketing materials is an outright lie).
  13. This was also brought up in the following topic, but there was no answer from IPS there and I do believe it is solvable: In almost all places where Item::$containerNodeClass is used, there is an isset test before accessing it and graceful fallback when it's not present. However, that check is missing in a few places that affect following and tagging. For followable: \IPS\core\Followed\Table:99 - this join should actually be within the preceding if statement (since I believe it only makes sense when permissions are used) core/front/table/tables/rows.phtml:104 - the if should also check that method_exists, as it the pattern everywhere else (but this isn't insurmountable because it will use the table desc if available) core/front/tables/manageFollowRow.phtml:43 - as above \IPS\Content\Search\Elastic\Query:467~487 - (what's going on with that code structure?) you should only honour $includeContainers if isset($class::$containerNodeClass) \IPS\Content\Search\Mysql\Query:416~436 - as above for tags: \IPS\Content\Item:7684 - should also check isset($containerClass) \IPS\Content\Item:7727 - I think this should be a constant if containerClass is empty, but I'm not sure how it's used so I may be wrong here. To my eye, all those changes should be feasible, and would make it much easier to avoid nasty hacks involving pseudo-nodes. nb. I searched for these occurences using intellij, so there may be places that I missed because the static analysis wasn't powerful enough.
  14. That's a gmail-specific thing, although some other email providers do something similar. It would not be safe to blanket strip dots from emails, because that could open up security vulnerabilities relating to emails that do distinguish dots.
  15. I have turned up the threshold for falling back to manual queries, because the vast majority of the time the queries that require manual action only take a couple of seconds to run (even with the threshold turned up). Maybe if I turn it down that will solve the issue, but instead it will mean I spend time copying queries into mysql-client instead. That raises another question though - can you fix the query runtime heuristic to be more accurate? UPDATE queries are pretty fast, as are DROP INDEX and I think DROP COLUMN, whereas CREATE INDEX queries are very slow, but they're all judged by the same threshold. Given the replies above though, as well as my memory of prior upgrades where I hadn't updated the threshold, resetting the threshold back to default is not the magic bullet here.
  16. If one request times out in the JS upgrader, it falls back to the considerably slower HTML-based upgrader. Could there not be some logic to retry the request a couple of times, ideally with a bit of backoff (if it's an nginx or transport layer error it's probably a timeout, and retrying will then hit the case where the task is already done), rather than making the upgrade experience suck? At worst, the HTML upgrader shouldn't have delays between redirects - as soon as the response comes back it is ready to receive another request. It's reasonably easy to work around by just deleting the mr parameter from the URL, but that relies on me paying attention to the upgrader.
  17. I don't say this much about this software, but the new ACP onboarding wizard is great. It has almost everything you need to bootstrap the community. However, it is missing settings to configure the ingoing and outgoing email addresses, and (less critically) the SMTP/etc settings, which means that emails default to being sent from the admin user's address (and therefore often don't get delivered).
  18. Why is "Logging into the front-end from a new device" so far away from "Logging into the front-end from a known device"? They should really be next to each other IMO (also, changing email/password/2fa settings would also make sense to be together). This is on a fresh 4.5.3.1 install.
  19. XRegExp is 135KB when minified as part of root_library.js (that whole bundle is only 383KB, so it's a significant chunk), but it's only used in two places: In ipsautolink/plugin.js, it's just used to evaluate a native JS regex, which I believe is totally unnecessary (use this.urlRegex.test(text) instead!) In ips.search.results.js, where it's just used to replace using a native JS regex (use $(this).text().replace(new RegExp(...), '...') instead, as you do for each subsequent replacement in that chain) (while you're at it, why does that code replace with HTML, HTML escape everything, then selectively unescape the content you just added?!) Removing those two places that don't, to my understanding, utilise the library in any meaningful way would allow you to slim down the site by a not-insignificant amount, and make certain people happy.
  20. Currently, there are no config options when following a member, so it's just binary following/not following. This means that you have to opt into all or nothing for being notified about new content posted by your followees - there's no way to follow a member for status updates only. It would make sense if the follow dialog when following a member let you choose between "status updates only" and "all content". Implementing this would be pretty straight forward I believe - just update the special casing of following members in \IPS\core\front\modules\system\notifications::follow to add another option for status updates only, then update the follow types checked when sending status notifications to include that type. Unfortunately the aforementioned follow method is basically unhookable because it constructs and uses the form in the same method without exposing it to hooks.
  21. I deleted a plugin's hook in the dev center, but it wasn't removed from hooks.json. It was caught by CI for me, but it's obviously not great.
  22. Needing an "Other Settings" panel in Account Settings is a bit of a red flag that maybe all of those settings-related things should be unified to make them more coherent. I see no reason for ignored users to be separate Notification settings has a slightly more complex UI, but I can't see any blockers to bringing that UI into settings, and doing so would bring it inline with other sites (GitHub, Stack Overflow, reddit, and iirc also facebook etc) Profile settings is more arguable because having it as a dialog when viewing your own profile is good, but the profile settings is also where I think the most benefit can be gained - users expect their signature settings to be in their profile, because that's where their other public-facing things are configured, and they expect to be able to change other things by going to settings. This probably needs more consideration of the tradeoffs, and maybe finding a neat solution, but my feeling at the moment is that unifying them would make sense.
  23. Similar to Stack Overflow, it would be neat if the checkForNewReplies behaviour extended to checking for edits to existing posts, probably as a button per post rather than in the existing toast.
  24. I noticed that in a few places you're hacking around support for lower case (http2) headers, and there are a bunch of places where you don't handle them correctly. Is there a reason that you're not lowercasing all headers when storing the headers (and also storing the originals for back compat)? eg there are a bunch of places where you test for $response->httpHeaders['Location'] || $response->httpHeaders['location'], but if you standardise them then you can just test one. You already standardise location, but you may as well do it for everything then deprecate the upper case ones.
  25. The marketplace onboarding flow is quite annoying when there are lots of non-marketplace apps/plugins to have to select "not in the marketplace" for each one, and to me it was non-obvious that I needed to do that. Ideally, I would like to be able to click continue without having clicked "not in the marketplace" in each one.
×
×
  • Create New...