|
Golbez posted:How recommended/good is it to shoehorn an existing site (and more importantly, an existing database schema) into a framework? It depends on how opinionated the framework is. Some will let you use only bits and pieces of their stuff, while others insist on using only their creations. If you can skip the data access layer / ORM bits, you should be OK. It's not going to be sane or feasible to hook up an existing database schema to an ORM without massive pain.
|
#
?
Sep 27, 2012 19:24
|
|
|
|
| # ? Jun 8, 2024 20:35 |
|
|
I'm a total PHP newbie. I understand the core concepts and am currently working on a site. What I'm wondering is where I should go to have my code critiqued/reviewed. Is this thread the right place for that?
|
|
#
?
Oct 2, 2012 22:18
|
|
|
Good a place as any.
|
|
#
?
Oct 3, 2012 05:27
|
|
|
caiman posted:I'm a total PHP newbie. I understand the core concepts and am currently working on a site. What I'm wondering is where I should go to have my code critiqued/reviewed. Is this thread the right place for that? Where are you learning from? You need to be aware that there is a lot of crap advice on the internet about how to program in PHP. But that said, if you post things in this thread I would imagine people will tell you if you are doing anything badly wrong.
|
|
#
?
Oct 3, 2012 05:34
|
|
|
How to avoid crap PHP advice: Ignore anything written before 2006 or that mentions PHP 5.0 is new. Ignore anything using the mysql_ family of functions. Ignore anything that doesn't mention what XSS or CSRF are.
|
|
#
?
Oct 3, 2012 05:48
|
|
|
I've mostly been teaching myself from this tutorial: http://www.tuxradar.com/practicalphp I'll post my code when I get home tonight.
|
|
#
?
Oct 3, 2012 15:05
|
|
|
caiman posted:I've mostly been teaching myself from this tutorial: http://www.tuxradar.com/practicalphp Uses the mysql_ family, but partially redeems itself by trying to do something sane. Unfortunately that sanity comes in the form of PEAR's DB class, which is awful and designed for PHP4. It speaks of register_globals and how to work around it not being there. The chapter on security doesn't mention anything about XSS, CSRF, etc. The optimization chapter contains hilarious cargo-cult voodoo that hasn't been relevant since PHP4. Conclusion: Your resource is crap. Run from it and never look back. McGlockenshire fucked around with this message at 18:19 on Oct 3, 2012 |
|
#
?
Oct 3, 2012 18:16
|
|
|
Would you mind elaborating on those criticisms, or point me somewhere that does? I'm especially curious why I shouldn't use mysql_ and what I should be using instead. EDIT: Furthermore, what is the recommended source for beginners learning PHP? Spatulater bro! fucked around with this message at 19:21 on Oct 3, 2012 |
|
#
?
Oct 3, 2012 19:07
|
|
|
caiman posted:Would you mind elaborating on those criticisms, or point me somewhere that does? I'm especially curious why I shouldn't use mysql_ and what I should be using instead. mysql_ doesn't include a function for preparing statements.
|
|
#
?
Oct 3, 2012 19:41
|
|
|
caiman posted:Would you mind elaborating on those criticisms, or point me somewhere that does? I'm especially curious why I shouldn't use mysql_ and what I should be using instead. Another reason why not to use mysql_ is the PHP manual itself: quote:Use of this extension is discouraged. Instead, the MySQLi or PDO_MySQL extension should be used. See also MySQL: choosing an API guide and related FAQ for more information. Alternatives to this function include:
|
|
#
?
Oct 3, 2012 20:09
|
|
|
Ok here's my code. Now, the one thing I know right off that bat that's wrong with it (aside from the mysql_ thing) is that there's WAY too much commenting. This was for my benefit to help teach myself as I went along and help avoid confusion as I read back through my code. What this page does in a nutshell: Presents a form for the user to register for the site. Some of the fields are validated. It allows the user to upload an avatar, then the code renames the file, moves it to a directory on the server, then re-sizes it and saves it as a .jpg. Once everything is okay it adds the new user to the database. code:
|
|
#
?
Oct 3, 2012 21:23
|
|
|
Quick notes: * Comments in PHP can just be // Comment, rather than /* Comment */ * import_request_variables() is deprecated in PHP 5.3 and removed in 5.4, and is unnecessary. * It seems usually better to use echo instead of print. * You seem to be running into a very common beginner mistake: Not knowing where to put the form logic. It's always a first instinct to write the form, then below it put the code to handle it. Then your brain twists inside itself and yells, "Wait, that's not how this works." The general flow for me is usually: PHP code:This may not sound like a big issue for this query, but for a login query it's fundamental. It also keeps people from deleting your database.
|
|
#
?
Oct 3, 2012 22:04
|
|
|
Golbez posted:Quick notes: I knew this one. I just chose my method 'cause I'm so used to doing it that way in css. Golbez posted:* import_request_variables() is deprecated in PHP 5.3 and removed in 5.4, and is unnecessary. So I simply remove that line? How do I quickly assign my POST variables the "var" prefix? Golbez posted:
I see. Is it a performance issue? Golbez posted:* You seem to be running into a very common beginner mistake: Not knowing where to put the form logic. It's always a first instinct to write the form, then below it put the code to handle it. Then your brain twists inside itself and yells, "Wait, that's not how this works." The general flow for me is usually: This is quite helpful. I've been unsure how to order things. Golbez posted:
I've known about mysql_real_escape_string but sorta forgot to implement it. Your explanation makes me realize how vital it is. Thanks for the feedback, it's exactly what I'm looking for.
|
|
#
?
Oct 3, 2012 22:42
|
|
|
caiman posted:So I simply remove that line? How do I quickly assign my POST variables the "var" prefix? quote:I see. Is it a performance issue?
|
|
#
?
Oct 3, 2012 23:10
|
|
|
I think your code could probably use more vertical whitespace, to make it easier to read. You don't have to go nuts, but a few blank lines here and there to break up the PHP code into logical chunks would make it much more readable.
|
|
#
?
Oct 4, 2012 02:24
|
|
|
If you only used prepared statements with either mysqli or PDO (I suggest PDO) you will not be susceptible to sql injection.
|
|
#
?
Oct 4, 2012 04:46
|
|
|
Question about PDO, if I want to do a query likecode:code:
|
|
#
?
Oct 4, 2012 06:03
|
|
|
code:
|
|
#
?
Oct 4, 2012 06:26
|
|
|
Mister Chief posted:
That looks like... something I was trying to grok a long time ago and couldn't make work, probably because I was still in an ADO frame of mind. So you can put in an array... would this work (sorry it's late here, can't fire up the test environment): code:
|
|
#
?
Oct 4, 2012 06:32
|
|
|
I don't see how it could.
|
|
#
?
Oct 4, 2012 07:39
|
|
|
PDOStatement::execute will take an array for binding multiple parameters at once, but it doesn't work like you are wanting it to for building the IN clause. I think you have to construct that portion of the query string yourself, and concatenate it in there.
|
|
#
?
Oct 4, 2012 07:44
|
|
|
You could use a little trickery to build your ON clause:php:<?
$STH = $DBH->prepare("SELECT * FROM :foo WHERE :column IN(:ar".implode(', :ar',array_keys($array)).")");
$STH->bindParam(':foo', $foo);
$STH->bindParam(':column', $column);
foreach ($array as $k=>$v) {
$STH->bindValue(':ar'.$k, $v);
}
$STH->execute();
$result = $STH->fetch();
$DBH = null;
?>
|
|
#
?
Oct 4, 2012 08:47
|
|
|
PDO is better than the alternatives but having lines of bindparam as the defacto usage pattern is jarring. Using reference passing instead of value? Really?? ugh. I've got some code that automatically 'explodes' arrays into sql lists and it does the same thing that bobthecheese has posted. But it's also little wrapper around PDO so I can only have to specify the bare minimum like: php:<?
$db = new Db();
$result = $db->query("SELECT * FROM :foo WHERE :column IN(:list)", array(
'foo' => 'users'
,'column' => 'id'
,'list' => array(1,4,9,33)
));
foreach($result as $row)
{
// hellooo nurse
}
?>
|
|
#
?
Oct 4, 2012 10:34
|
|
|
I agree that extending PDO to have your own query function makes coding easy as hell and is something that everyone should do. But I think you're going too far with automatically parsing arrays. Since most people use $_GET['id'] instead of using filter_input() with the FILTER_REQUIRE_SCALAR flag, someone is liable to pass an array as input when you'd expect a scalar, thus formulating an invalid query such as SELECT * FROM users WHERE id = '123', '456';. I'm still trying to figure out if this would be vulnerable to some type of injection though. Maybe it's not a big deal, since it'll just throw an error if someone purposely puts in invalid input. But potentially you could gently caress up a strangely written INSERT command and write into arbitrary columns. Or maybe if you have two inputs in a row, one you set as a blank array, and the second as an array with two elements. That's might let you bypass any bounds checking code on the first variable. For IN clauses, I eventually settled on this. It's not the prettiest, but I think it's the best option code:I don't really think any solution to this is particularly nice. I've considered making a $DB->buildInStr() method, but you end up having to write a whole bunch of code everywhere just to be able to correctly handle these requirements: 1) work if a regular array is passed 2) be able to build a NOT IN() query or (big sql code instead of just a column name) IN (blah) 3) correctly handle if the array is empty (maybe by writing WHERE 1=0 when a blank array is passed) Even things like CodeIgniter's active record does it wrong. where_in() will build an invalid query and throw a database error if the array is blank. Basically, PDO doesnt have this ability, and no matter how you try to do it, it's going to be ugly, and has the possibility of making your input query string unstable, which kinda breaks the use of PDO. Peanut and the Gang fucked around with this message at 14:12 on Oct 4, 2012 |
|
#
?
Oct 4, 2012 14:02
|
|
|
So we started getting this error on our website today: Fatal error: require() [function.require]: Failed opening required '../Sorting Systems/sortingheader.php' (include_path='.;C:\php5\pear') in D:\Hosting\2630349\html\Products\Sorting-Systems\sorting.php on line 43 It happened out of no where with nothing changing as far as us putting any new files or changing any directories. Talked with the host provider, they said they had upgraded their php to 5.3 and we were still on 5.2, so we did the upgrade and it was still broken the line 43 it is refering to is this <?php require("../Sorting Systems/sortingheader.php"); ?> Did some function change in the php upgrade? I am at a complete loss here. Flaggy fucked around with this message at 15:38 on Oct 4, 2012 |
|
#
?
Oct 4, 2012 15:34
|
|
|
Flaggy posted:So we started getting this error on our website today: This may work: <?php require(__DIR__."../Sorting Systems/sortingheader.php"); ?>
|
|
#
?
Oct 4, 2012 15:39
|
|
|
Knyteguy posted:This may work: <?php require(__DIR__."../Sorting Systems/sortingheader.php"); ?> Still get the same error.
|
|
#
?
Oct 4, 2012 16:22
|
|
|
Flaggy posted:Still get the same error. Try php:<?php require(__DIR__."/../Sorting Systems/sortingheader.php"); ?>
|
|
#
?
Oct 4, 2012 16:45
|
|
|
musclecoder posted:Try Tried that as well, still getting the same error, bad week to quit smoking this is frustrating.
|
|
#
?
Oct 4, 2012 17:13
|
|
|
The folder name has a dash where the require() command assumes a space.
|
|
#
?
Oct 4, 2012 18:20
|
|
|
barbarianbob posted:I agree that extending PDO to have your own query function makes coding easy as hell and is something that everyone should do. But I think you're going too far with automatically parsing arrays. Since most people use $_GET['id'] instead of using filter_input() with the FILTER_REQUIRE_SCALAR flag, someone is liable to pass an array as input when you'd expect a scalar, thus formulating an invalid query such as SELECT * FROM users WHERE id = '123', '456';. I'm still trying to figure out if this would be vulnerable to some type of injection though. Maybe it's not a big deal, since it'll just throw an error if someone purposely puts in invalid input. But potentially you could gently caress up a strangely written INSERT command and write into arbitrary columns. Or maybe if you have two inputs in a row, one you set as a blank array, and the second as an array with two elements. That's might let you bypass any bounds checking code on the first variable. Generate :bind_names for every array element, you doofus!  php:<?
// look for any array and flatten it into an sql list
foreach($bind as $name=>$bind_value)
{
if(is_array($bind_value))
{
$added_parameters = array();
// you cannot bind multiple values to one parameter; we have to add parameters to the query
// TODO make this also work robustly with positional parameters (these things: ? )
$new_name = $name;
if($name[0] !== ':') $new_name = ':'.$name;
$count = 1;
foreach($bind_value as $v) $added_parameters[$new_name.($count++)] = $v;
$q = preg_replace('/\(*'.quotemeta($new_name).'\)*/', //find :param or (:param)
'('.join(',',array_keys($added_parameters)).')', //replace with: (:param1,:param2, ...)
$q);
// remove old parameter that we used as a template
unset($bind[$name]);
$bind = array_merge($bind, $added_parameters);
}
}
?>php:<? $q = "select * from :table where id in (:ids)"; $bind = array( 'table' => 'users' ,'ids' => array(5,3,9,2) ); // turns into this $q = "select * from :table where id in (:ids1,:ids2,:ids3,:ids4)"; $bind = array( ':table' => 'users' ,':ids1' => 5 ,':ids2' => 3 ,':ids3' => 9 ,':ids4' => 2 ); // so you can pipe them though PDO like so: $statement = A_PDO_OBJECT->prepare($q); $statement->execute($bind); ?>
|
|
#
?
Oct 4, 2012 18:37
|
|
|
KARMA! posted:The folder name has a dash where the require() command assumes a space. I tried putting a dash in the name as well, everywhere it was supposed to be but I am still getting that error after I put it back in the folder. And still getting the same error I really do appreciate the help.
|
|
#
?
Oct 4, 2012 18:48
|
|
|
KARMA! posted:Generate :bind_names for every array element, you doofus! He might have to deal with arrays of various lengths, you doofus! edit: oh. You attempted to deal with that. But your solution doesn't get around the fact that you have to assemble a query based on the data you're going to send, when the basic idea of parameterised queries is that you should have a query that is set in stone and then you bind values for a particular case. If you have to create the query on-the-fly based on the number of parameters then that's not ideal. Hammerite fucked around with this message at 19:08 on Oct 4, 2012 |
|
#
?
Oct 4, 2012 19:05
|
|
|
Wow, I thought I had a simple question. Good to see it's causing some discussion. I wonder if making a query like "... WHERE ((`column`=:arrayval1) OR (`column`=:arrayval2) OR (`column`=:arrayvalN))" would be better or different in any way. Probably not, and harder on the DB I suppose.KARMA! posted:Generate :bind_names for every array element, you doofus! I have never seem quotemeta() before, is that better/different than preg_quote()?
|
|
#
?
Oct 4, 2012 19:22
|
|
|
Flaggy posted:I tried putting a dash in the name as well, everywhere it was supposed to be but I am still getting that error after I put it back in the folder. And still getting the same error I really do appreciate the help. I'm sorry man, this is really hard to debug over the wire. All I can suggest is to a exit(__DIR__); and see if that actually returns D:\Hosting\2630349\html\Products\Sorting-Systems. Is that's the case, then welp.  Hammerite posted:He might have to deal with arrays of various lengths, you doofus! But you're not sending a query plan ID of some sort to the database so that it can go "hey I don't need to parse the SQL to know what you want, thanks man". You're ALWAYS going to send SQL in a string that the database has to parse! The only thing the database can do speed-wise is keep a query plan cache of recently parsed (parameterized queries), but then it's trivial to store an arraylength amount of query plans instead of 1. If you really want to get speedy results you might need to look into stored procedures, index tuning, and such. To me, the biggest help in using a wrapper around PDOs is fullproof escaping and enabling me to write as little code as possible to get whereever I need to go.
|
|
#
?
Oct 4, 2012 19:40
|
|
|
FeloniousDrunk posted:Wow, I thought I had a simple question. Good to see it's causing some discussion. I wonder if making a query like "... WHERE ((`column`=:arrayval1) OR (`column`=:arrayval2) OR (`column`=:arrayvalN))" would be better or different in any way. Probably not, and harder on the DB I suppose. Any database worth its salt would come up with the same queryplan (the steps a database would take to get you a result). the ... IN (...) Is clearer to read however, which is pretty important for us with a meat brain. FeloniousDrunk posted:I have never seem quotemeta() before, is that better/different than preg_quote()? To be honest I found quotemeta when I searched for a suitable function and never looked any further . preg_quote's a better option.Although that does make me wonder what characters are allowed as a parameter name and whether I should automatically filter those or let PDO error out... hmmm...
|
|
#
?
Oct 4, 2012 19:59
|
|
|
Finally got it fixed, thanks guys you saved my day. It was basically the /../ thing, which took a bit of fixing. Thanks again.
Flaggy fucked around with this message at 21:26 on Oct 4, 2012 |
|
#
?
Oct 4, 2012 21:23
|
|
|
Golbez posted:
If I remember right, "echo" is very very very very very very very slightly faster than "print" -- trivial. There's also some other bullshit that this StackOverflow question explains. However, in general, for basic poo poo like echo "Some poo poo here."; use whatever works best. I did a lot of stuff in QBASIC when I was younger and "print" got stuck into my brain, so it's what I end up using more often when I'm not mixing them up. Short version: The only difference you're really likely to run into as a sane developer is that you can do "... or print('yyy')", but not "... or echo('yyy')".
|
|
#
?
Oct 6, 2012 23:14
|
|
|
Hopefully I didn't miss too many things here, but does anyone see any vulnerabilities in this code besides bot spamming vulnerability?php:<?php /*PHP Mailer Anthony Hill 10/7/12 */ // Get their email if(isset($_GET['email'])) { $email = $_GET['email']; } else { echo 'No e-mail set'; exit; } // Fill these in // $to is your e-mail address that you want people to contact you on // Example: $to = "youremail@gmail.com" $to = ""; // $from should preferably be an e-mail on your website to help messages get past spam filters $from = ""; /* Don't modify below this point if you don't know what you're doing */ $subject = "$email has contacted you"; $website = $_GET['website']; $name = $_GET['name']; $headers = "From: $from" . "\r\n" . "Reply-To: $email" . "\r\n" . 'X-Mailer: PHP/' . phpversion(); // Validate if(!filter_var("$email", FILTER_VALIDATE_EMAIL)) { echo 'Please enter a valid email address.'; exit; } // Make sure the message isn't null if($_GET['message'] != '') { $message = "E-Mail: $email\r\nWebsite: $website\r\nContact Name: $name\r\n\r\nMessage:\r\n"; $message .= $_GET['message']; } else { echo 'Please enter a valid message.'; exit; } echo 'Your e-mail has been sent'; mail($to, $subject, $message, $headers); ?> code:E: Mostly XSS or something. I'm not extremely familiar with anything besides SQL injection. Knyteguy fucked around with this message at 06:38 on Oct 8, 2012 |
|
#
?
Oct 8, 2012 06:34
|
|
|
|
| # ? Jun 8, 2024 20:35 |
|
|
Stick the creation of the headers after the email format validation. Nearly wrote an essay on how header injection works. The built-in filter is OK, but use isemail if you can. It's so RFC compliant that they had to file an erratum just to clear things up. Looks fine otherwise, with the exception of the whole spambot thing. Watch out, your host might not let you send using an arbitrary From, and doing so will get that mail dropped in the spam folder if the From's domain tries to implement SPF or DKIM. McGlockenshire fucked around with this message at 07:07 on Oct 8, 2012 |
|
#
?
Oct 8, 2012 07:04
|
|