4 ms·

Can someone explain to a newbie why this code is so bad? Reading through it it seemed to generally make sense and not be too complicated.

ceejayoz 7y ago
It's not really bad for the time it was written, but today you wouldn't want to write PHP code like this. Lots of global variables that can clobber each other (if one bit of code, even in one of the includes, redefines $user, everything explodes), no classes, no code autoloader...
- cosmotic 7y ago
  How are classes are a sign of superior code? How is an autoloader a sign of superior code? Keep in mind, this is old PHP and those things were new or nonexistent.
munk-a 7y ago
The lack of `chroot`[1] makes me sad off the bat - for some reason that function seems like a secret, everyone actually wants to use it (or wanted to before autoloading became as easy as it is) but nobody did. Additionally I'd love to see that file split up into smaller chunks simply to lower the scope of thought. It looks like nearly all of those function calls are modifying variables passed by reference instead of resolving the value out via `return` this isn't bad and is indistinguishable at a technical level in terms of functionality, but it's a kind of horrible approach from expressability. They're doing things with datetime that are unsafe and wrong (like assuming 246060 is the number of seconds in a day) but people getting datetime logic wrong is as old as... well time. Oh, and you've got some pretty bizarre looking function signatures - I'm sure there is a reason for this but I'd want to ask some questions about this one... $permissions = privacy_get_reduced_network_permissions($user, $user); It's possible to go through this and nitpick a bunch of stuff, it looks like it's mostly just an older style though. The big problems aren't here though... I'm not seeing any reads into $_POST (and `param_get_slashed` looks like a nice function for sanitizing input) - additionally, I'm not seeing a single line of SQL nor am I seeing any memcached calls, so the data access layer may already be well isolated architecturally. 1. https://www.php.net/manual/en/function.chroot.php https://www.php.net/manual/en/function.chroot.php
- duskwuff 7y ago
  `chroot()` has no place in a web application. The system call requires the process to be running as root.
  - voltagex_ 7y ago
    Can you call that and then drop permissions?
    - duskwuff 7y ago
      In theory, yes. But that's still bad, because it means that a nontrivial amount of your application code (as well as whatever is launching it, like the PHP-FPM server or the web server) is running as root.
zelly 7y ago
It's decent as far as 2000s era PHP goes. If you want to poke your eyes out, look at the source of some of the PHP web forum software out there. The leaked code ignores pretty much all design patterns and software architecture norms. But not having to deal with that, and being able to cobble something together in a hurry, was the appeal of PHP. You can always rewrite later. Nowadays Facebook's landing page is served by highly tuned binaries compiled from C++. If they decided in 2007 to start out with that, we probably would not know who Zuckerberg is.
- iudqnolq 7y ago
  It seems to me (with absolutely no experience) that architecture and design patterns are more trouble than they're worth for code in the low thousands of LoC, which this appears to be.

ceejayoz 7y ago

It's not really bad for the time it was written, but today you wouldn't want to write PHP code like this. Lots of global variables that can clobber each other (if one bit of code, even in one of the includes, redefines $user, everything explodes), no classes, no code autoloader...

cosmotic 7y ago
How are classes are a sign of superior code? How is an autoloader a sign of superior code? Keep in mind, this is old PHP and those things were new or nonexistent.

munk-a 7y ago

The lack of `chroot`[1] makes me sad off the bat - for some reason that function seems like a secret, everyone actually wants to use it (or wanted to before autoloading became as easy as it is) but nobody did. Additionally I'd love to see that file split up into smaller chunks simply to lower the scope of thought. It looks like nearly all of those function calls are modifying variables passed by reference instead of resolving the value out via `return` this isn't bad and is indistinguishable at a technical level in terms of functionality, but it's a kind of horrible approach from expressability. They're doing things with datetime that are unsafe and wrong (like assuming 246060 is the number of seconds in a day) but people getting datetime logic wrong is as old as... well time. Oh, and you've got some pretty bizarre looking function signatures - I'm sure there is a reason for this but I'd want to ask some questions about this one... $permissions = privacy_get_reduced_network_permissions($user, $user); It's possible to go through this and nitpick a bunch of stuff, it looks like it's mostly just an older style though. The big problems aren't here though... I'm not seeing any reads into $_POST (and `param_get_slashed` looks like a nice function for sanitizing input) - additionally, I'm not seeing a single line of SQL nor am I seeing any memcached calls, so the data access layer may already be well isolated architecturally. 1. https://www.php.net/manual/en/function.chroot.php https://www.php.net/manual/en/function.chroot.php

duskwuff 7y ago
`chroot()` has no place in a web application. The system call requires the process to be running as root.
- voltagex_ 7y ago
  Can you call that and then drop permissions?
  - duskwuff 7y ago
    In theory, yes. But that's still bad, because it means that a nontrivial amount of your application code (as well as whatever is launching it, like the PHP-FPM server or the web server) is running as root.

zelly 7y ago

It's decent as far as 2000s era PHP goes. If you want to poke your eyes out, look at the source of some of the PHP web forum software out there. The leaked code ignores pretty much all design patterns and software architecture norms. But not having to deal with that, and being able to cobble something together in a hurry, was the appeal of PHP. You can always rewrite later. Nowadays Facebook's landing page is served by highly tuned binaries compiled from C++. If they decided in 2007 to start out with that, we probably would not know who Zuckerberg is.

iudqnolq 7y ago
It seems to me (with absolutely no experience) that architecture and design patterns are more trouble than they're worth for code in the low thousands of LoC, which this appears to be.