Colonel_mortis

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.

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.

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.

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.

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.

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.

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.

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.

I may have misunderstood what you mean by this, but I can't seem to find any way to install a plugin from the files without the XML, IN_DEV or not. Writing the hook data to disk is great though, and if nothing else the XML file can at least now be reconstructed.

I want to check my code into git, and be able to get started on a new install fairly easily by just cloning it. This lends itself well to collaborative environments, being able to easily recover from scrapping and restarting development environment, and potentially even CI workflows. I also currently deploy my code using git. For apps, it's great. I just clone my repo, click an install button in ACP, and I'm good to go. For plugins though, it really sucks: Hook information isn't stored in a file anywhere, so in order to check in enough information to reinstall the plugin I would need to download the xml, copy it back to my plugin dir, then commit that too Then I get those annoying uncaught exception handler blocks added in my dev environment For that reason, plugins can only be installed through the "upload xml" workflow, so installing a dev version means installing the xml then finding it in disk and adding the dev files separately Hook class names are install-specific, so once I get it installed in a new environment I end up with a bunch of git changes - this makes it impossible to collaborate with them Plugin names are case insensitive and get lower-cased when xmled, which looks ugly when you've installed a camel-cased plugin. There's also no option to make them snake- or kebab-cased because of the arbitrary alphanumeric restriction For these reasons I've been trying to centralise my plugins into apps, but that loses the flexibility of having small plugins that just do what they say on the tin, without needing to worry about a place for a settings UI for one or two options. That solution is obviously untenable for plugins that aren't just being developed for a single site though. If you implemented these fairly simple changes (hooks.json file, install from disk, custom names for hook classes, retain case and allow -_ in plugin names), I would be much happier when developing small changes like this. I think all of these things can be implemented in a backwards-compatible way (for hooks.json, if not present fall back to creating it based on the contents of the DB), so I can dream that my complaints haven't missed the 4.5 boat.

If a user posts a poll and sets it not to display the names of the people who voted for each option, members don't get warned that their vote will be public and they may wish to share their opinion anonymously. However, the author of the poll can just edit the poll to set it to public, and see the names and choices of everyone who has already voted. This defeats the point of having private polls. Either it shouldn't be possible to change a poll from private to public, or votes from when it was private should be anonymised.

It would actually be super easy to implement - they just need to remove the check for author != loggedIn()->member_id in checkForNewReplies (just removing part of a single line). The only other consequence of doing this is that you'll get "1 new reply" toasts for your own replies if you have it open in multiple tabs/browsers, but I think that isn't a bad thing.

It would be great if there was a user option to only receive email notifications when they're offline, or to get a summary email of the notifications that they've missed if they don't visit the site for some period of time (as Stack Overflow does). There's no point getting email notifications when you're actively browsing the site, but equally if you're only an infrequent visitor you don't want to miss any replies to things that you're interested in or that you posted, and something like this would strike that balance (while also helping to boost retention in a user-friendly way).

At the moment if there is a video embedded in the editor, it can be difficult to remove because clicking it just controls the video. If it was a widget and had a grab handle, it would be a bit easier to control.

My furl.json entry is "status_view": { "friendly": "status/{#id}", "real": "app=statusupdates&module=status&controller=view", "verify": "\\IPS\\statusupdates\\Status", "seoPagination": true } but this causes a redirect loop when trying to access /status/1/page/2/ because it doesn't remove the page url correctly when normalising it to check that it matches the canonical version - normally the page part is matched in the {?} seo title variable when finding the furl to normalise with, but there isn't one available here. There must already be logic somewhere which copes with page path params because it does identify the furl template correctly in most places, just not when finding the template to normalise for equality. I have worked around it by adding "alias": "status/{#id}/{!}", to catch the paginated version, but this isn't ideal.

I don't disagree, and I think it makes sense. However, I would dispute your claim that it requires a new dns lookup, tcp connection and tls setup - it is fetching from the same server as the main content so if your server is correctly configured with keep-alive it will reuse the existing connection, resulting in 0ms overhead. Even if you have keep-alive disabled (as ips did for a while, although they may have changed that now), the dns lookup will always have been cached and if you are running an up to date version of openssl and nginx/apache you can get 0- or 1-rtt tls session resumption, so there should only be 1-2 round trips of overhead. Resending the cookies is still a reasonable concern, but the others are not.

Agreed, especially as complying with user requests to delete accounts is required for compliance with the GDPR.

Actually, never mind, turns out I was looking at the wrong column, and it is taking an insignificant amount of time. Sorry about that.

rules_rules doesn't have any indexes, and is queried a few times with where parameters.

If dev mode is enabled, the app breaks the site because an E_NOTICE is emitted by line 435 of your ipsPatternsActiveRecord hook (__set) when you try to get an undefined index from _data (because it's being set for the first time). You should probably add an array_key_exists or isset check there to prevent that error. Also, can you add some indexes to the Rules tables? The queries are currently fairly slow, and adding an index should help quite a lot.

4.1.7

Editing an administrative profile field using the edit profile form on a member's profile (front not ACP) doesn't save.