F**k the SMF project. F**k it up its stupid a**. <-- says Arantor ;)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: Fuck the SMF project. Fuck it up its stupid ass.
« Reply #15, on March 15th, 2013, 04:45 PM »
Oh, I'm waiting until Saturday because all hell's going to break loose then. Or more accurately, depending on who does what, shit will fly.

Some of you may know that Labradoodle has been working on his Menu Editor Pro mod. I'd believed for a long time it was vapourware but it actually exists. So I bought a copy, directly from him (refusing to give vbgamer a cut, of course) for the list price. And he should know I don't need it or intend to use it but to review it.

I sent him my comments, of which there were a great many negative ones, and I told him I was going to give him three days to figure out what to do about it, before I made it public. Then yesterday I made a comment about the mod, about how it doesn't work properly with SimpleDesk even though it claims to, plus doesn't work as far as I can see with Dream Portal, TinyPortal or PortaMX or Aeva for that matter, and he deleted it. I can't wait to see whether he will delete the post on Saturday or not. But I've already spoken to a couple of the moderators there.

See, here's the problem, and it's firmly one of his own making, as far as I'm concerned. If you're making a paid mod (especially one that's quite expensive, especially given that there are free mods that do most of the same thing), it should be your best work. But he's an SMF dev, so that's the standard of quality I'm going to hold him up to - because if he isn't writing at that level, he has no business being an SMF dev. And paid mods are usually a fairly good indication of the quality someone's going to turn out generally. And I'm not impressed in the slightest. There's a fair amount of reinventing the wheel here, and not for a good reason.
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
The way it's meant to be

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: Fuck the SMF project. Fuck it up its stupid ass.
« Reply #17, on March 15th, 2013, 05:00 PM »
He made 5,000 lines of mod to make a nice interface around editing that array. Including such gems as a 50 line easter egg loaded every page, and his own file caching system that insists on pretty-printing everything, including comments, to a file, as opposed to, say, using serialize.

In fact, lemme dig out all my comments. I tried to treat it exactly as I would a conventional mod review: study the code first then reviewing it in use.



modifications.xml
1. QueryString edit need not be an actual edit, it should have been tied to the integrate_buffer hook.

2. I'm presuming the first Subs edit is to make sure it's after the menu hook? You could always do what SimpleSEF does and check the position of the hook periodically and if it isn't the last entry, adjust the hooks.
The only advantage is that anyone who added their code after the main code (like SimpleDesk) might be caught by it.

3. Why not just ditch the first edit to Subs and wrap everything up in the end of the menu handler and be done with it?
This would mean you would have one edit (and only one edit) but you'd know you'd nail every single item, hook or otherwise, just do a bit more clean-up on it.

4. With the index.php edit, exactly the same deal, do it on the actions hook, and do it last, you can even fix the hook check for any hooks you do use if you want to.
That said, I can see the logic of doing it there, in the hopes of cleaning up any mods that injected after the main array - but there are very few of those.


*install_mep_hooks.php
There is a part of me that wants to recommend a single file, and whether it's installing or uninstalling is based on whether it finds the mod's files.
Certainly it would mean that if you add new or change hooks later, it's one less thing to remember to maintain.


integrate.php
theLegacy() - yay for easter eggs. Amusing comments are fine, but big easter eggs of 50 lines of code, especially one that loads *every page* is not clever.

Also, integrate.php is included with integrate_pre_include, but I see at least it's included in the code modification. I'd also prefer to see the admin hook function not to be loaded every page load - every bit of parsing is important.


global.php
Seems to me this could easily be in the main code. Kind of surprised Aeva isn't in there.

Also a bit annoyed at the way SD support is handled. If it exists, Subs-SimpleDesk.php will already be loaded, no need to reinclude it again, just check for the function existence, will do the same job without the performance of a non-firing require_once.

The last bit in global.php could be streamlined to:

Code: [Select]
if ($deny_security === false && !$response)
{
if (!empty($button['membergroups']))
{
$these_membergroups = explode(',', $button['membergroups']);
foreach ($these_membergroups as $value)
if (in_array($value, $user_info['groups']))
return true;
}
}
return false;

There's other various minor optimisations like that - if something starts out as false, then through some process becomes true, and can't ever become false again, you might as well exit as soon as you know it's true, rather than slogging through multiple loops whose outcome won't change it any further. Also, no need to pull the keys if you're not going to use them.

If you really wanted to go nuts, you could convert that to a count(array_intersect($these_membergroups, $user_info['groups'])) > 0 but I think you'd have to be careful about type juggling.


portal-compat.php
Hmm, interesting methods of detecting whether portals are active or not, but I'll come back to that later on. Any reason TinyPortal, PortaMX and Dream Portal are excluded?


default-logs.php
I know you're listing default actions, but linking to easter eggs? about:*, modifycat, and clock are all easter eggs. I'd also discourage linking to viewquery since it's really not that useful as a menu item. In fact linking to it directly will have unintended consequences unless you're providing a query id number.


tasks.php
Why have you used your own cache? Aside from a security risk, there is a performance limit, since it's now I/O bound as opposed to being able to use memcache or similar memory cache.

If you are going to cache anything, at least look at SMF's code, serialize, addslashes, output. No need to artificially fluff the writing of the cache, also no need to make the cache bigger just to be more readable - no-one should be looking at it directly. Incidentally that makes it more likely to trip the bug that meant file_put_contents even needs LOCK_EX in the first place. And you really don't need to be dumping comments in the cache file. It's not meant to be human readable. Go look at SMF's to see how, generally, it should be done.

Also, WTF is '01001001010011000100110001001101' and why is it used as a literal value in a query without proper escaping? (Line 103. Should be pushed to a {string:var} and imported that way, not just escaped with inline backslashes, since SMF doesn't support a {literal:value} type) Hmm, that looks like ASCII... ILLM. That wouldn't be a typo on the LILM you normally dedicate your mods to, would it?

Another example of streamlining would be the switch in lines 244 onwards. Better would be to declare an array up front:

Code: [Select]
$frame_targets = array(
1 => '_parent',
2 => '_blank',
3 => '_top',
);

Then in place of the switch:
Code: [Select]
$temp['final_target'] = isset($frame_targets[$row['target']]) ? $frame_targets[$row['target']] : '_self';

Of course we'd declare the array outside the where specifically so that we don't have to redeclare it every iteration.

Similarly, the internal action three-way branch could be simplified to:
Code: [Select]
//-- Internal Action
$internal_action = '';
if ($row['link_type'] == 1 || $row['link_type'] == 3)
$internal_action = $row['href'];

On top of that, you have lists of items already imploded, which you store in the cache as such. Save yourself some time, explode that to an array, typecast all the members to ints then save it as part of the serialized array directly. It doesn't save *quite* as much room in the cache file but it'll be faster than exploding it when you come to parse it later.


OK, so now I'm actually going to try it.

Yay for an extra query, every single page, to check the packages table to see if I've suddenly installed a portal - which would be fine if I installed it through the package manager. Also, why no caching? I hear you're a big fan of that. I also hear you don't understand how to do it properly especially on a very large site where caching is absolutely critical.

Modifying buttons
1. Can't add a button just for 'regular members'.

2. Please tell me you didn't add jQuery just for slow animation effects. And if you are going to have animations, no more than 200ms for elements people want to edit (for what we do, we found 150ms for expanding panels and 200ms for closing worked best)

3. Do you really need to dump every single one of your language strings into the page? In fact, you're going to have *so* much fun with that, because json_encode is not standard even in 5.2 (WordPress had to include compatiblity functions for it even when they went 5.3 only), and the best part, json_encode doesn't even work properly on non-UTF-8 strings. It's not a problem for English, of course, but anything else it will be.

4. The Associated Permissions icon on a button seems very ambiguous, especially for admin where it's not obviously controlled by a permission, e.g the admin button is shown as not having an associated permission except that it actually doesn't just rely on group access ordinarily, it uses array('admin_forum', 'manage_boards', 'manage_permissions', 'moderate_forum', 'manage_membergroups', 'manage_bans', 'send_mail', 'edit_news', 'manage_attachments', 'manage_smileys') in fact. SP at least also adds its own to that, as well as SD doing its own thing.

5. The shiny 'Your changes have been saved' thing at the top... it would be cool if it were AJAXively saving changes, but to do it with animation when you're coming back from another page load just looks tacky.

6. Layout is buggy when SimpleDesk is enabled, when using a screen of 1000px wide, the tickbox overlaps onto the 'Button Name' textbox.

7. In fact, the SD integration doesn't work. The blue box says it will automatically be set to active when SD is enabled, or that I can enable the helpdesk button from Display Options... except it is enabled by default (the option is to turn it off), and no combination of pressing things in the SD options area allows me to re-show it. Oh, and it dumps the submenu too, both for the admin and the helpdesk items, some of which might be useful, but will need to be recreated from scratch - as will any other mods with dynamic menu items.

8. Now that I look at it again, the first time it builds the action cache, it's actually incorrect. Even if SD is installed and running (and active) then install MEP, it still doesn't let me add the button until I clear the cache. The same seems to hold true of any other mods I've tried.

9. Um, why does restoration not show anything when you first install?

10. More disturbingly for something 'so advanced and customisable', I now realise why Aeva and SD menus aren't supported... unless it fits the SMF permissions model, there's actually no way to add a button. For example Aeva's tab doesn't have any children unless there's some new items. But there's no way to configure it, which means it isn't 'completely customisable'. It's 'completely customisable provided it doesn't do anything that doesn't exactly fit SMF'.


Honestly? It feels like a beta version, not a 1.0.0 version, and certainly not from someone who has been trusted with the keys to a major platform. I go on about that but it's important. If you wrote code like this for the SMF trunk, you would get criticised for poor coding style, as quite rightly you should. If you're telling me that 5076 lines of your own code (which is what I make the code count, excluding comments and blank lines, plus excluding jQuery because you didn't write that) took you 3 years and this is the result, you have further to go than I thought.
Posted: March 15th, 2013, 04:53 PM

And remember, this is being written by a core SMF developer as a paid mod. You see now why it bothers me so much?

godboko71

  • Fence accomplished!
  • Hello
  • Posts: 361
Thank you,
Boko

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: Fuck the SMF project. Fuck it up its stupid ass.
« Reply #20, on March 15th, 2013, 05:12 PM »
Exactly my point. Had he not been on the SMF dev team, I'd have given him a lot less stick for it. Still wouldn't have been ideal, but I would have been nicer about some of the aspects of it.

Also, did I mention he claims to be on the SD dev team, though I have yet to see anything he's actually done with it except add a bug.
Quote from Dragooon on March 15th, 2013, 05:10 PM
Still reading the thing but I'd remove the code, just not to give him any ground.
That's my rewrite of his code. There is no chance it's going to cause issues. If anything I'd rather it stay because then people can ask what the original was and how much cruftier it is.

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
Re: Fuck the SMF project. Fuck it up its stupid ass.
« Reply #21, on March 15th, 2013, 05:16 PM »
Honestly, had I not known you I would've thought you're making half of that stuff up :P.
Quote
3. Do you really need to dump every single one of your language strings into the page? In fact, you're going to have *so* much fun with that, because json_encode is not standard even in 5.2 (WordPress had to include compatiblity functions for it even when they went 5.3 only), and the best part, json_encode doesn't even work properly on non-UTF-8 strings. It's not a problem for English, of course, but anything else it will be.
How does wedge handle json + UTF-8? I didn't know this but I am using json_encode for some wedge stuff so I'm not sure now.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: Fuck the SMF project. Fuck it up its stupid ass.
« Reply #22, on March 15th, 2013, 05:19 PM »
That's something that was mentioned between me and the SMF moderators when he removed the small post I made - if what I have to say is full of crap, he can refute it publicly. But if it's legit, then he can defend himself publicly, not through deleting my posts.

json_encode is absolutely fine on UTF-8, assuming it's available. It's only for non English ISO stuff that there's a problem, assuming it's available. There is a compat function kicking around for that use.

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

tumbleweed

  • Posts: 7
Re: Fuck the SMF project. Fuck it up its stupid ass.
« Reply #29, on March 15th, 2013, 05:48 PM »
I had been looking every now and then into the thread since FrizzleFried had deleted his response. Guess I missed your first one.
Going to hit the "notify" button. See what happens...