Understanding php error handling and code execution

Question

I am trying to port a php application to .net and while reviewing the php code I came across a snippet which is a bit confusing. The block shown below

$cur_xml_valid = 'N';                        
$GenerateXml[] = "<J>";
$seq ++;
$GenerateXml[] = "<ANumber>" . $seq . "</ANumber>";
$GenerateXml[] = "<Customer>" . str_replace(array('&','>','<','"','\''), array('&amp;','&gt;','&lt;','&quot;','&apos;'), $invoice['customer']) . "</Customer>";
$GenerateXml[] = "<CompletionDate>" . str_replace(' ', 'T', $invoice['DateCreated']) . "</CompletionDate>";
$GenerateXml[] = "<OrderNumber>" . $invoice['SO'] . "</OrderNumber>";
$GenerateXml[] = "<OrderType>" . $invoice['type'] . "</OrderType>";
$GenerateXml[] = "<Comments>" . str_replace(array('&','>','<','"','\''), array('&amp;','&gt;','&lt;','&quot;','&apos;'), $invoice['Comments']) . "</Comments>";
$cur_xml_valid = 'Y';

if ($cur_xml_valid == 'N') {
    $sqlxmlmismatch = "INSERT INTO xml_log (No,LogDesc) VALUES ( '$invoice_no','DOES NOT EXIST IN FILES (" . $invoice['Address'] . ")')";
    $resultxmlmismatch = $db->query($sqlxmlmismatch);
    $objWorksheet->getCell("BH$start_row")->setValue('DOES NOT EXIST IN FILES - NOT SENT');
}

Initially $cur_xml_valid is set to 'N' and after building up some xml string it is then set to 'Y'. Right after an if condition evaluates whether $cur_xml_valid is 'N' or not.

Q. Please confirm this is just bad code and $cur_xml_valid will never set to 'Y' if an error occurs. The whole execution stops in php if an error occurs, assuming there is no try/catch blocks?

Q. For a normal flow (without an exception) $cur_xml_valid will always get set to 'Y'.

100% bad code. It's open to SQL injection - and the $cur_xml_valid thing doesn't make sense either.. (that I can see) — treyBake
– treyBake, Commented Sep 24, 2019 at 15:43
@treyBake thanks i thought so too, just not quite familiar with php :) — John
– John, Commented Sep 24, 2019 at 15:45
@bfris Reviewing code not written by the author/maintainer is off-topic on Code Review — user11523568
– user11523568, Commented Sep 24, 2019 at 15:54

Barmar · Accepted Answer · 2019-09-24 15:46:50Z

2

Your suppositions are correct, there's no way for $cur_xml_valid to be N when it gets to the if statement.

To give the coder the benefit of the doubt, there might have been an earlier version that had validation code in the top section (perhaps it got the XML from an API). The dynamic XML fetching and validation has been removed, but they left the variable and the code that checks it. This kind of thing is not uncommon as code evolves.

answered Sep 24, 2019 at 15:46

Barmar

789k57 gold badges555 silver badges669 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

John Over a year ago

that makes perfect sense, could have been left there as the code evolved like you suggested. Thanks!

Collectives™ on Stack Overflow

Understanding php error handling and code execution

1 Answer 1

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related