SquirrelMail's PHP code

Daniel_Beardsmore

Need I say more?

belgariontheking

Apparently you do.

elgate

@belgariontheking said:

Apparently you do.

I find your ideas intriguing and which to subscribe to your newsletter.

AbbydonKrafts

OMGWTFBBQ!

Seriously, though. Not all of us have gone dumpster-diving in others' PHP code. Perhaps you'd like to enlighten those of us who haven't seen the innards of SquirrelMail with a few choice snippets?

MasterPlanSoftware

Would it be too much to write a simple sentence to let us in on WTF you are talking about?

Maybe a link or something?

Avenger

I don't get it. I looked in their SVN and saw well-commented and structured code, with a slight excess of commented-out blocks with notes indicating their purpose.

Did I randomly pick the best files out of the project or is there something else that needs explaining?

Daniel_Beardsmore

I am not sure where to begin. One of the oddest things is functions which are not defined in directly referenced code. left_main.php (woot frames) calls displayHtmlHeader, but nothing it requires actually defines this function, so where do I look to find out to use it? A grep reveals it to be in functions/page_header.php, but how does left_main know about it? (There is in fact a require_once for it, but it's commented out; which is slightly missing the point of require_once.)

The reliance on global variables is also fun. For example:

 function formatMailboxName($imapConnection, $box_array) {

     global $folder_prefix, $trash_folder, $sent_folder,
            $color, $move_to_sent, $move_to_trash,
            $unseen_notify, $unseen_type, $collapse_folders,
            $draft_folder, $save_as_draft,
            $use_special_folder_color;

Where do they come from? Who set them? Are they related in blocks or is this just corralling a bunch of random settings? Lots of functions make reference to a slew of global variables that came from goodness knows where. For example, to choose a custom stylesheet for the current page – which in fact zaps the theme's default – just set $custom_css somewhere, and displayHtmlHeader will probe this variable automatically.

There's plenty of the standard fare -- HTML and PHP mixed up so badly and running on for pages such that you can't figure out what anything is doing, how, or where; indecision on whether we're using HTML 3.2 or 4, so we have complex, tangled code to worry about setting HTML 3.2-era display properties and <font> tags as well as setting styles directly on every tag one by one, at the same time as being designed to (badly) handle global stylesheets.

One of the most spectacularly useless calls has to be in here:

 echo '<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">' .
          "\n\n" . html_tag( 'html' ,'' , '', '', '' ) . "\n<head>\n";

Apparently we need to call a special made-up function to write "<html>". Note that that DOCTYPE is dangerously invalid (missing the DTD schema URI causes some painfully odd behaviour in browsers), which goes along well with nonsense like <a ...><div ...><p ...>...

It's not as bad as some of the front-page material, but it's quite a mess. I also appreciate how all the PHP code is stashed in src/, so whatever URL you choose to run this from, the URL ends up with src/ added to the path, which is where the frameset script also lives.

Avenger

Okay, you win. But at least it isn't written in Perl ...

Daniel_Beardsmore

Now now, I like Perl! (The Perl codebase I've inherited on is nowhere near this insane, with clearly defined classes to separate out the code by logic, database, HTML generation, HTML snippets etc, and lots of templates.)

This code is full of all sorts of inanities though. The weirdest bit is the apparent IMAP feature, "Let this folder contain subfolders" when creating a new IMAP folder. I've never seen that before in any other IMAP client, and I can create subfolders in a folder where I left it unticked. Someone having a giraffe?

morbiuswilters

@Daniel Beardsmore said:

I am not sure where to begin. One of the oddest things is functions which are not defined in directly referenced code. left_main.php (woot frames) calls displayHtmlHeader, but nothing it requires actually defines this function, so where do I look to find out to use it? A grep reveals it to be in functions/page_header.php, but how does left_main know about it? (There is in fact a require_once for it, but it's commented out; which is slightly missing the point of require_once.)

So a file is included by another function and this blows your mind? And obviously grepping for the function definition was really difficult. I also don't understand your comment about require_once -- it's meant to include a file only once. How is having it commented out a big deal?

@Daniel Beardsmore said:

The reliance on global variables is also fun. For example:

 function formatMailboxName($imapConnection, $box_array) {

     global $folder_prefix, $trash_folder, $sent_folder,
            $color, $move_to_sent, $move_to_trash,
            $unseen_notify, $unseen_type, $collapse_folders,
            $draft_folder, $save_as_draft,
            $use_special_folder_color;

Eh, global variables are quite fine. However, these should be part of an object or associative array.

@Daniel Beardsmore said:

This code is full of all sorts of inanities though. The weirdest bit is
the apparent IMAP feature, "Let this folder contain subfolders" when
creating a new IMAP folder. I've never seen that before in any other
IMAP client, and I can create subfolders in a folder where I left it
unticked. Someone having a giraffe?

This is part of the IMAP spec, which you are obviously unfamiliar with. However, IMAP is a real bit of garbage anyway and most servers and clients don't implement every feature, at least correctly.

I've been through SquirrelMail's source many times and it's kind of messy but it's a very old app and it's meant to run on PHP 3, I believe. The use of frames came way before AJAX was common. Mostly, there are better projects that have surpassed SquirrelMail, but it could definitely use an update. I guess it is a little bit of a WTF, but it's a small and old project that helped many a sysadmin get a webmail system up in a hurry, so I respect it for that, at least.

Daniel_Beardsmore

@morbiuswilters said:

So a file is included by another function and this blows your mind?

My mind is not blown, but why would anyone do that? Generally any time you #include/import/require/etc., that file contains or references all the functions you need. You're not supposed to have to recursively read through lots more files to figure out where functions are coming from. Funnily enough, I've just remembered that I've actually done the same thing with my some of own projects (and I'm not sure why), but I can see now that it's rather strange. Most recently, though, I've given functions a naming scheme so that I can tell where they're defined, although similarly you can use classes and objects to clarify.

@morbiuswilters said:

And obviously grepping for the function definition was really difficult.

It's still nice to know how the codebase slots together and where everything comes from.

@morbiuswilters said:

I also don't understand your comment about require_once -- it's meant to include a file only once. How is having it commented out a big deal?

Uncommented, it would say "I am using this include file" to the developer reading it, but nevertheless do nothing as the file was already included somewhere. It's actually a problem, as $theme_css can't actually be set at runtime, as something somewhere is overriding it. Being global, it just "exists" with no indication of where it came from, and where you can and cannot actually go about setting it. I've decided to write a new way to set the page's active stylesheets for now, so I have some control.

@morbiuswilters said:

Eh, global variables are quite fine.

When used wisely, yes. They're also better off on the outside of library code, not on the inside. In this case, it's impossible to tell where the global variables that the library code uses, are actually coming from, and sometimes if you try to set them right before a call, it still fails. All arguments to a function in another code file should be explicit.

morbiuswilters

@Daniel Beardsmore said:

My mind is not blown, but why would anyone do that? Generally any time you #include/import/require/etc., that file contains or references all the functions you need. You're not supposed to have to recursively read through lots more files to figure out where functions are coming from. Funnily enough, I've just remembered that I've actually done the same thing with my some of own projects (and I'm not sure why), but I can see now that it's rather strange. Most recently, though, I've given functions a naming scheme so that I can tell where they're defined, although similarly you can use classes and objects to clarify.

It makes a lot more sense to include dependencies where they are used and not at the highest level. Things chain together then and you're not writing the same 50 require statements at the top of every script. This is pretty common with perl and PHP and I'm amazed that this is the first time you've come across it and that you would rather manually specify every dependency all the way down.

@Daniel Beardsmore said:

When used wisely, yes. They're also better off on the outside of library code, not on the inside. In this case, it's impossible to tell where the global variables that the library code uses, are actually coming from, and sometimes if you try to set them right before a call, it still fails. All arguments to a function in another code file should be explicit.

I understand this, I just don't think you should rail against global variables when the real problem is global variables that are not accessed consistently or in an easy to determine manner.

Cap_n_Steve

Have they made it so the navigation frame actually refreshes automatically yet? It's pretty annoying to have to manually refresh it to see the updated totals or empty your trash.

pauluskc

Very true. SM is WTF'able. I do think that with all this massive knowledge, why not implement a new free open-source webmail application? I would assume that a more updated IMAP-capable client is available, and if not, why not write one since the standard is so familiar? Cuz you don't wanna. So deal with what was chosen, or make the change. I've had pretty much no pain ever with changing IMAP "webmail" applications. Like a good abstraction layer, thusly exists IMAP/POP/SMTP/etc.

<semi-sarcastic nerding>

As much as I hate typing it every time (thanks keyboard shortcuts/autocomplete) I use the old fashioned $GLOBALS array every time. Pretty obvious where that's from. Those other super globals also autocomplete rather well and so I use them too. Also, I usually have a nice common library. Then sub- and end-libraries. In the very end libraries (which usually follow a specific naming convention) I typically don't leave a cookie-crumb back to the start. That's what the name is for. And I like to eat cookies.

</just saying>