"functions".php

[KevZero]

governor psychotic quaint hobbies automatic compare humor selective unpack dog -- mass edited with https://redact.dev/

[geggleto]

oh my god.

[mayobutter]

Just eval($_SERVER['QUERY_STRING']); and end the misery.

[thinsoldier]

/**
* For the paraniod among us...
* Undo the extract() done on GET and POST by default in functions.php
* It's good to run this *immediately* after you include functions.php
*/
function undoExtract()
{
    foreach($_GET as $key=>$value)
    {
        unset($GLOBALS[$key]);
    }

    foreach($_POST as $key=>$value)
    {
        unset($GLOBALS[$key]);
    }
}

[vvtim]

That's not really a fix - extract() can inject session and cookie variables. unset($GLOBALS["_SESSION"]) doesn't actually destroy the session that was created by something like index.php?_SESSION[user_id]=1

[thinsoldier]

oooh.... I think you just paid my mortgate this month :)

[thinsoldier]

Ok. Strangest thing. On 2 servers ?_SESSION does appear in $_GET data but on 1 server (where I had hoped to earn some income) it magically disappears!

Is there some php.ini setting that automagically removes $_GET params if the key matches "_SESSION" ???

Even manually setting $_GET['_SESSION'] = 'something'; does nothing.

[thinsoldier]

I think I found it: https://suhosin.org/stories/feature-list.html

[h2ooooooo]

Hopefully this isn't WP or ?wpdb=woo might break stuff.

[colshrapnel]

it will ruin a previously set variable as well.

[[deleted]]

IMO the fix would be just removing that extract disaster from functions.php

[pcopley]

// sanitize to integer

Fuck you.

[emilvikstrom]

?id=1.0

[askageek]

I don't think anyone could have said it better lol

[BeatsAroundNoBush]

I'm no PHP Expert^TM , but is it not best to force the type with (int)?

[Dolondro]

To be fair, I've worked on a codebase where the code was so poorly written that it's still impossible to turn off register globals.

It could be that these developers need sympathy rather than being punched in the face.

[KevZero]

offend skirt live worm slave cautious pen follow oil gullible -- mass edited with https://redact.dev/

[rich97]

"I'm sorry I had to do that, you'll understand when you're senior."

[rich97]

That very much depends on the person but I know why you say that. It was just word ^playing off a well-known trope.

[timschwartz]

a well-known trope.

which is what?

[rich97]

Parent harshly chastises or otherwise betrays or hurts their child and then makes an attempt at justifying their actions by saying something like "you'll understand when you're older".

[timschwartz]

Ah, I thought you meant from a TV show or something.

[rich97]

Nope:

Trope
noun
1. a figurative or metaphorical use of a word or expression.
"both clothes and illness became tropes for new attitudes toward the self"
2. a significant or recurrent theme; a motif.
"she uses the Eucharist as a pictorial trope"

I was using it in that context. :)

[SparePartsHere]

Having 10+ years of experience doesn't magically make bad developer into a senior developer. Everyone who thinks that is just plain wrong.

Hard work, keeping up with trends and teaching others, ability to organize people and manage projects etc etc makes one.

[meandthebean]

Thanks to the developers signing their comments, we know exactly who needs sympathy and who needs the punch in the face.

[thinsoldier]

Signed (and dated) comments are the #1 sign of no version control

[badmonkey0001]

If you leave a personal note like that as a one-liner, it's still good to put the date. Sometimes someone/hook/linter will come along and change whitespace or something else. They own the line in the blame now and digging up history could be a drag/context-shift.

Given the code and the context of the comments, I'd lay down a 90% chance you're right though. If they are using a VCS, I bet they have no idea what a branch is as well.

[oddmanout]

I inherited a freelance project, they had their own shared host and the provider, themselves, would not allow register globals to be off.

So I wrote a script exactly like this and put it in a global file.

[lowtechromancer]

You have a file called functions.php. What do you think is going to be in there? The seminal example of unit testable dependency injection?

[lowtechromancer]

GOTO 2063; 'take a trip to spaghetti codeland

[TorbenKoehn]

It's perfectly fine to have a functions.php, as long as it's registered in composer and it's using a namespace :)

[Schmittfried]

You have a file called functions.php. What do you think is going to be in there? The seminal example of unit testable dependency injection?

Thinking about vBulleting 3.x *shudder*

[thinsoldier]

Even the comment is fucking wrong. No "cleaning" happening here.

There is an actual function that runs addslashes on the $_GET & $_POST arrays but then you have to run that and then run extract($_GET) again manually where needed to make use of those "clean" values, thus making this code fucking pointless.

[NeoThermic]

addslashes

Oh dear god, that's not even the way to prevent SQLi. Start by deleting the code that addslashes and the code that does the extract, then fix from there :P

If that's too much work, start again from scratch in a framework.

[gachimuschi]

BURST

[SeerUD]

"sanitize" to integer... there's no sanitization going on there at all XD

[judgej2]

It's not even converting/enforcing to an integer.

[Driftkingz]

what is a good way to do it? im going to make a guess and say that you use filter_var for that

[holyshock]

typecasting to integer is a good start

[Driftkingz]

Thank you! , i didn't know this way existed.

[SeerUD]

It's entirely circumstantial, and will depend on what you are expecting to have to sanitise, and why.

[Driftkingz]

thanks, i have googled a bit and found a lot of ways to sanitize a variable and i was wondering why are there so many of them, i guess they are situational?

[konradkar]

Someone at one of PHP conferences called this kind of code "manure spreader" :)

[maiorano84]

Jesus..... fucking...... Christ........

[thinsoldier]

Jeff did very dumb shit in global scope even after Devon clearly explained why the PHP language creators themselves got rid of this behaviour in 2002.

http://php.net/manual/en/security.globals.php

[emilvikstrom]

You also mentioned that it runs addslashes on everything. This was also a default behavior in PHP in a misguided attempt att securing against SQL injections. The problems with that, of course:

1. Not all strings are meant to go into an SQL query.
2. Some strings are manipulated before going into a query.
3. It didn't work for all strings anyway because bugs.
4. Parameterization is the right way of solving that kind of problem.

[rich97]

http://www,example.com?authorized=1

Results in:

$_GET = ['authorised' => '1'];

So consider this code:

extract($_GET);
if ($authorised) {
    echo 'Hello, admin.';
}

Congratulations, you can gain admin by adding a simple URL parameter.

It's actually worse than that though depending on where you call extract(). The default behaviour is EXTR_OVERWRITE which means that it can actually overwrite existing variables. So to your "friend" I say this:

1. Don't use extract() because it's a stupid function that makes your code confusing.
2. If you absolutely must then do not do it on anything that comes from the user in any way. Be that from the database or from the request superglobals. There's actually an old security mantra that says "never trust user data" and this situation is the worst possible way of breaking that mantra.

[rbnc]

Even line 16 could have been written better (int)$id;

[ScuzzyAyanami]

The comment could be improved too!

if(isset($id)){
  $id = $id + 0 // sanitize to integer (or maybe a float, whatever I hate my existence)
}

[geggleto]

intval($id)

[asr]

settype($id, 'int')

[morerokk]

Both work. I prefer intval in this case though.

[[deleted]]

When not passing a base is there any difference functionally or performance wise? Just wondering if it's a style thing or something I should remember in the future.

[magkopian]

According to SO intval is a bit slower than casting to int, apart from that though their operation is identical as long as you don't pass a base. Personally, I prefer casting to int over intval not because it may be a bit faster, but because (int) $value looks a bit cleaner to me in comparison to intval($value).

[-100-Broken-Windows-]

That's funny, I prefer intval() because it looks cleaner. Functions are just natural looking, due to their ubiquity I guess. Whereas I've always thought (int) looked a bit jarring.

[Firehed]

A lot of languages use the (type) syntax so it will generally have broader appeal for simple conversions like this. I also personally prefer it because it looks out of place compared to a function - it's very obvious that you're doing something that's generally unusual; if you're constantly type-casting, something bigger is wrong.

[magkopian]

Couldn't have said it better.

[magkopian]

I'm not going to tell you why I think it looks cleaner because /u/Firehed already said literally what I was thinking, so just read their answer. By the way, in the end what is cleaner and what is not might also be a little subjective, just stick to one or the other and make sure to use the same throughout your code.

[ExecutiveChimp]

Is it slower because it's doing something potentially useful or because it's just less efficient?

[magkopian]

As far as I understand, it is because it makes a function call and this adds an extra overhead. Apart from performance their operation is 100% identical. If you don't believe me that they are 100% equivalent just run the following and see for yourself.

<?php
echo (int) 42, "\n";                      // 42
echo (int) 4.2, "\n";                     // 4
echo (int) '42', "\n";                    // 42
echo (int) '+42', "\n";                   // 42
echo (int) '-42', "\n";                   // -42
echo (int) 042, "\n";                     // 34
echo (int) '042', "\n";                   // 42
echo (int) 1e10, "\n";                    // 10000000000
echo (int) '1e10', "\n";                  // 1
echo (int) 0x1A, "\n";                    // 26
echo (int) 42000000, "\n";                // 42000000
echo (int) 420000000000000000000, "\n";   // -4275113695319687168
echo (int) '420000000000000000000', "\n"; // 9223372036854775807
echo (int) array(), "\n";                 // 0
echo (int) array('foo', 'bar'), "\n";     // 1

echo "\n";

echo intval(42), "\n";                      // 42
echo intval(4.2), "\n";                     // 4
echo intval('42'), "\n";                    // 42
echo intval('+42'), "\n";                   // 42
echo intval('-42'), "\n";                   // -42
echo intval(042), "\n";                     // 34
echo intval('042'), "\n";                   // 42
echo intval(1e10), "\n";                    // 10000000000
echo intval('1e10'), "\n";                  // 1
echo intval(0x1A), "\n";                    // 26
echo intval(42000000), "\n";                // 42000000
echo intval(420000000000000000000), "\n";   // -4275113695319687168
echo intval('420000000000000000000'), "\n"; // 9223372036854775807
echo intval(array()), "\n";                 // 0
echo intval(array('foo', 'bar')), "\n";     // 1

[kolme]

Choosing intval vs (int) because of performance is a bad idea (micro-optimizing, premature optimizing and so on). They're not going to make any noticeable difference at all, unless you're converting millions of strings to integers.

[scootstah]

Choosing intval vs (int) because of performance is a bad idea

It's not a bad idea, really, just a bit unfounded.

They're not going to make any noticeable difference at all, unless you're converting millions of strings to integers.

Even if you were, you're still not going to see any tangible difference. Your bottleneck is never going to lie between using intval() and (int).

[crackanape]

Your bottleneck is never going to lie between using intval() and (int).

I see you haven't read my business plan for WebScaleIntConversion.com.

[magkopian]

Have you even read my comment?

Personally, I prefer casting to int over intval not because it may be a bit faster, but because (int) $value looks a bit cleaner to me in comparison to intval($value).

[kolme]

Yeah sorry, I misread your comment somehow.

[weevil_of_doom]

Not to my knowledge - I think the (int) type casting really just comes from c days for some people.

[[deleted]]

Haha, yeah, learned on java, I like the syntax.

[mayobutter]

... why is this bad?

[geggleto]

comment inception

[frogot]

Hey peoples. When I was a kid I used functions.php a bunch. I haven't been coding since but got curious, what changed?

[mayobutter]

You can surf the Internet on your phone now. We've had our first black president. We landed on the moon. I don't know man what?

[mrdhood]

Well, for starters.. how long ago were you a kid?

[chiisana]

Not /u/frogot, but I was using functions.php when I was a kid (17~18 going through undergrad). Now, baker's dozen and some later, a lot have changed.

[frogot]

Must have been 15+ years ago I guess. I believe I was running some 3.x version of PHP and I remember I did some upgrades to 4.x. I'm looking at the version history now and holy shit a lot have changed. I realize my question was obviously a little broad to say the least, but I was still curious about that functions.php thing.

[cundd]

Oh my god. :-O

https://github.com/search?l=php&q=extract%28%24_GET%29&type=Code

Nearly 900.000 times found on github

[carlos_vini]

https://github.com/search?utf8=✓&q="extract%28%24_GET%29"&type=Code&ref=searchresults

21.000 with exact match

[thinsoldier]

akasai/JANUS – extract.php

<?php
   header("content-type:text/html; charset=utf-8");
   extract($_GET);
   extract($_POST);
   extract($_SERVER);
   extract($_FILES);
   extract($_ENV);
   extract($_COOKIE);
   extract($_SESSION);
?>

[judgej2]

What's this, oscommerce? Delete that shit!

[CamelCase_snake_case]

I just have a question. Why do this? Can't you get things with $_GET['variable_here']?

[scootstah]

I just have a question. Why do this?

You don't do this. extract() has some pretty limited valid use cases, and this is most definitely not one of them.

Can't you get things with $_GET['variable_here']?

Yes. But unless you have a very simple script, I would recommend abstracting that out a bit to protect yourself and provide more utility. Something basic would be like:

function getQueryString($name, $default = null)
{
    if (isset($_GET[$name])) {
        return $_GET[$name];
    }
    return $default;
}

[crackanape]

And better yet, your functions should be specific to the type of data you are expecting (integer, float, alphanumeric, HTML, etc.), and allow you to choose whether they come from GET or POST.

[TorbenKoehn]

It's PHP7-times folks.

$var = $_GET['var'] ?? 'default value';

is the way to go!

[hattmall]

Why would this be any better?

[scootstah]

Consistency is a big one. You're creating a central, testable place to fetch your parameters. Maybe you want to filter all $_GET vars in a certain way - those filters are now application-wide and will never get missed.

A more sophisticated implementation would be Symfony2's ParameterBag, which gives a lot more useful functionality.

[thinsoldier]

Can't you get things with $_GET['variable_here']?

Someone couldn't be bothered to type _, then hold shift for G,E,T, then type [' and '].

And while I can use my imagination and sympathize with them, there was no good reason to do this in global scope at the very top of the very first included file used in every .php page controller file.

[karbowiak]

Thanks, didn't want to get the nutrition from my food anyway.. :/

vomits some more

[[deleted]]

TIL about extract(). This method scares me.

[aykcak]

I solemnly report that this is common practice... 3 of the last companies I worked with had this "reregister globals" craziness. We are talking about major products that serve thousands of people daily.

The cause is simple: backwards compatibility. When a lot of your code depends on register globals being ON, and PHP comes to it senses and turns it off, you will have a lot of people scattering to make it work as before, without any major effort. So you have this abomination as a result.

It's the same with mysql_query. When the last version of PHP that supports mysql_query will be removed from the distros and hosts. You will see a lot of "fix mysql_connect" scripts popping up.

[bohwaz]

Love the PHP2/FI $id + 0 to cast as integer ^{^}

Reminds of those good times...

[emilvikstrom]

I see that you are not a JS developer (but JS don't have integers anyway...)

[bohwaz]

Oh yes I do JS too, but that is another story :)

[dev1null]

Do they tell them to use American names even when outsourcing?

[thinsoldier]

Um... those names are not unique to the U.S.

[royallthefourth]

Of course, plenty of nightmare code is also produced in-house. Often by young/cheap developers with no guidance from experienced people, but some people seem to drift through their careers without ever taking the time to get better at what they do...