fix: PHP 8.x compatibility - count() on null, mysqli_query() args, session_start() duplicate#809
Closed
ocjorge wants to merge 6 commits into
Closed
fix: PHP 8.x compatibility - count() on null, mysqli_query() args, session_start() duplicate#809ocjorge wants to merge 6 commits into
ocjorge wants to merge 6 commits into
Conversation
- AJAXInterface.php: Fix duplicate session_start() using session_status() check - ZipLookup.php: Initialize variables before use, guard foreach against null, add isset() checks - CareerPortal.php: Wrap PHPMailer send in try-catch to prevent fatal error when mail not configured - candidates/Add.tpl: Add is_array() check before accessing parsingStatus array - candidates/Search.tpl: Add isset() check for keySkills key - candidates/Show.tpl: Cast extraFieldRS to array in count() calls - candidates/Edit.tpl: Cast extraFieldRS to array in count() calls - companies/Show.tpl: Add is_array() guards for contactsRS, departmentsRS; cast extraFieldRS - companies/Edit.tpl: Cast extraFieldRS to array in count() calls - companies/Add.tpl: Cast extraFieldRS to array in count() calls - contacts/Show.tpl: Cast extraFieldRS to array in count() calls - contacts/Edit.tpl: Cast extraFieldRS to array in count() calls - contacts/Add.tpl: Cast extraFieldRS to array in count() calls - joborders/Show.tpl: Cast extraFieldRS to array in count() calls - joborders/Edit.tpl: Cast extraFieldRS to array in count() calls - joborders/Add.tpl: Cast extraFieldRS to array in count() calls - home/Home.tpl: Add is_array() guard for placedRS - install/backupDB.php: Fix mysqli_query() argument order (PHP 8 changed parameter order) - careers/CareersUI.php: Remove useCookie dependency for registration block display - rss/index.php: Define LEGACY_ROOT constant if not already defined Tested on PHP 8.4.21 + MariaDB 10.11 on Debian 12
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens several legacy PHP templates/utilities against runtime warnings (e.g., count() on non-arrays), adjusts some table column ordering, and adds a few defensive guards around optional functionality (parsing, email, sessions).
Changes:
- Added array casting / array checks before
count()and loops across multiple templates. - Fixed
mysqli_query()parameter order and added a safersession_start()guard. - Refactored
ZipLookupand gated resume parsing / parsing UI based on license status.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| rss/index.php | Defines LEGACY_ROOT defensively before including legacy libs. |
| modules/joborders/Show.tpl | Casts extra fields dataset for safer count(). |
| modules/joborders/Edit.tpl | Casts extra fields dataset for safer count() in loop. |
| modules/joborders/Add.tpl | Casts extra fields dataset for safer count() in loop. |
| modules/install/backupDB.php | Fixes mysqli_query($connection, $sql) call signature. |
| modules/home/Home.tpl | Adds array guard before counting $placedRS. |
| modules/contacts/Show.tpl | Casts extra fields dataset for safer count(); reorders “Entered By” column. |
| modules/contacts/Edit.tpl | Marks Title with *; casts extra fields dataset for safer count() in loop. |
| modules/contacts/Add.tpl | Marks Title with *; casts extra fields dataset for safer count() in loop. |
| modules/companies/Show.tpl | Adds array guards before count(); adjusts activity table column order. |
| modules/companies/Edit.tpl | Casts extra fields dataset for safer count() in loop. |
| modules/companies/Add.tpl | Casts extra fields dataset for safer count() in loop. |
| modules/careers/CareersUI.php | Gates parsing behavior/UI behind LicenseUtility::isParsingEnabled(). |
| modules/candidates/Show.tpl | Casts extra fields dataset for safer count(); reorders “Entered By” column. |
| modules/candidates/Search.tpl | Avoids undefined index for keySkills. |
| modules/candidates/Edit.tpl | Casts extra fields dataset for safer count() in loop. |
| modules/candidates/Add.tpl | Adds parsing-limit messaging/UI conditions; casts extra fields dataset for safer count() in loop. |
| lib/ZipLookup.php | Replaces/removes prior methods with lookupZip() using Google geocode XML lookup. |
| lib/CareerPortal.php | Catches mail exceptions (currently silent). |
| lib/AJAXInterface.php | Avoids calling session_start() when a session is already active. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </tr> | ||
|
|
||
| <?php for ($i = (intval(count($this->extraFieldRS))/2); $i < (count($this->extraFieldRS)); $i++): ?> | ||
| <?php for ($i = (intval(count($this->extraFieldRS))/2); $i < (count((array)$this->extraFieldRS)); $i++): ?> |
| <!-- CONTACT INFO --> | ||
|
|
||
| <?php for ($i = (intval(count($this->extraFieldRS))/2); $i < (count($this->extraFieldRS)); $i++): ?> | ||
| <?php for ($i = (intval(count($this->extraFieldRS))/2); $i < (count((array)$this->extraFieldRS)); $i++): ?> |
| </tr> | ||
|
|
||
| <?php for ($i = (intval(count($this->extraFieldRS))/2); $i < (count($this->extraFieldRS)); $i++): ?> | ||
| <?php for ($i = (intval(count($this->extraFieldRS))/2); $i < (count((array)$this->extraFieldRS)); $i++): ?> |
| </table> | ||
|
|
||
| <?php if (!count($this->placedRS)): ?> | ||
| <?php if (!is_array($this->placedRS) || !count($this->placedRS)): ?> |
Comment on lines
+125
to
+130
| <?php if (PARSING_ENABLED && | ||
| count($this->parsingStatus) && | ||
| $this->parsingStatus['parseUsed'] >= $this->parsingStatus['parseLimit'] && | ||
| $this->parsingStatus['parseLimit'] >= 0): ?> | ||
| <a href="http://www.catsone.com/professional" target="_blank">All daily resume imports used. For more, upgrade to CATS professional</a>. | ||
| <?php endif; ?> |
| <nobr> <?php /* FIXME: remove nobr stuff */ ?> | ||
| <?php if (isset($this->overAttachmentQuota)): ?> | ||
| <span style="font-size:10px;">(You have already reached your limit of <?php echo(FREE_ACCOUNT_SIZE/1024); ?> MB of attachments, and cannot add additional file attachments.)<br /></font>Copy and Paste Resume: | ||
| <span style="font-size:10px;">(You have already reached your limit of <?php echo(FREE_ACCOUNT_SIZE/1024); ?> MB of attachments, and cannot add additional file attachments without upgrading to CATS Professional Hosted.)<br /></font>Copy and Paste Resume: |
Comment on lines
+17
to
+20
| $sUrl = 'http://maps.googleapis.com/maps/api/geocode/xml?sensor=false&address='; | ||
|
|
||
| if ($zip != '') { | ||
| $oXml = @simplexml_load_file($sUrl . $zip); |
Comment on lines
+461
to
+471
| try { | ||
| $mailer = new Mailer($this->_siteID, $userID); | ||
| $mailerStatus = $mailer->sendToOne( | ||
| array($destination, ''), | ||
| $subject, | ||
| $body, | ||
| true | ||
| ); | ||
| } catch (Exception $e) { | ||
| // Mail not configured - fail silently | ||
| } |
| </td> | ||
| <td class="tdData"> | ||
| <input type="text" name="title" id="title" class="inputbox" style="width: 150px" /> | ||
| <input type="text" name="title" id="title" class="inputbox" style="width: 150px" /> * |
- ZipLookup.php: Restore public API (getCityStateByZip, makeSearchableUSZip, getDistanceFromPointQuery) as compatibility wrappers; switch Google Maps URL to HTTPS; remove error suppression (@) - CareerPortal.php: Log mail exceptions via error_log() instead of silently ignoring - candidates/Show.tpl: Apply (array) cast consistently in loop start index - companies/Show.tpl: Apply (array) cast consistently in loop start index - contacts/Show.tpl: Apply (array) cast consistently in loop start index
ocjorge
changed the title
- candidates/Add.tpl: Change catsone.com link to HTTPS - candidates/Add.tpl: Fix mismatched </font> tag to </span> - home/Home.tpl: Use is_countable() instead of is_array() for Countable compatibility
Comment on lines
+27
to
+30
| $sUrl = 'https://maps.googleapis.com/maps/api/geocode/xml?sensor=false&address='; | ||
|
|
||
| if ($zip != '') { | ||
| if (($oXml = simplexml_load_file($sUrl . $zip))) { | ||
| foreach($oXml->result->address_component as $value) { | ||
| if ($value->type == 'route') { | ||
| $aAddress[1] = (string) $value->long_name; | ||
| } | ||
| if ($value->type[0] == 'postal_town') { | ||
| $loc_level_1 = (string) $value->long_name; | ||
| } | ||
| if ($value->type[0] == 'locality') { | ||
| $loc_level_1 = (string) $value->long_name; | ||
| } | ||
| if ($value->type[0] == 'administrative_area_level_1') { | ||
| $loc_level_2 = (string) $value->long_name; | ||
| } | ||
| if ($value->type[0] == 'administrative_area_level_2') { | ||
| $loc_level_3 = (string) $value->long_name; | ||
| } | ||
| if ($value->type[0] == 'country') { | ||
| $loc_level_4 = (string) $value->long_name; | ||
| } | ||
| } | ||
| } else { | ||
| $aAddress[0] = 1; | ||
| } | ||
| } else { | ||
| $aAddress[0] = 2; | ||
| } | ||
| if ($zip != '') { | ||
| $oXml = simplexml_load_file($sUrl . $zip); |
Comment on lines
+87
to
90
| <?php for ($i = 0; $i < intval(count((array)$this->extraFieldRS)/2); $i++): ?> | ||
| <tr> | ||
| <td class="vertical"><?php $this->_($this->extraFieldRS[$i]['fieldName']); ?>:</td> | ||
| <td class="data"><?php echo($this->extraFieldRS[$i]['display']); ?></td> |
Comment on lines
+125
to
+128
| <?php if (PARSING_ENABLED && | ||
| count($this->parsingStatus) && | ||
| $this->parsingStatus['parseUsed'] >= $this->parsingStatus['parseLimit'] && | ||
| $this->parsingStatus['parseLimit'] >= 0): ?> |
Comment on lines
+461
to
+472
| try { | ||
| $mailer = new Mailer($this->_siteID, $userID); | ||
| $mailerStatus = $mailer->sendToOne( | ||
| array($destination, ''), | ||
| $subject, | ||
| $body, | ||
| true | ||
| ); | ||
| } catch (Exception $e) { | ||
| // Mail not configured or failed - log error for debugging | ||
| error_log('OpenCATS Mailer error (site=' . $this->_siteID . '): ' . $e->getMessage()); | ||
| } |
- ZipLookup.php: rawurlencode zip before URL concatenation to prevent injection - ZipLookup.php: restore getDistanceFromPointQuery() returning expected select/join keys - CareerPortal.php: set mailerStatus=false in catch block for deterministic status - candidates/Add.tpl: add is_countable() guard before count() on parsingStatus
- Extract address component parsing to private parseAddressComponents() - Use early returns to reduce nesting - Reduces cyclomatic complexity from 14 to under 10 (Codacy threshold)
| //based on kilometers = (3958*3.1415926*sqrt(($lat2-$lat1)*($lat2-$lat1) + cos($lat2/57.29578)*cos($lat1/57.29578)*($lon2-$lon1)*($lon2-$lon1))/180); | ||
|
|
||
| // Legacy wrapper - returns expected select/join keys for distance filtering | ||
| $select = "(3958*3.1415926*sqrt((zipcode_searching.lat-zipcode_record.lat)*(zipcode_searching.lat-zipcode_record.lat) + cos(zipcode_searching.lat/57.29578)*cos(zipcode_record.lat/57.29578)*(zipcode_searching.lng-zipcode_record.lng)*(zipcode_searching.lng-zipcode_record.lng))/180) as distance_km"; |
|
|
||
| // Legacy wrapper - returns expected select/join keys for distance filtering | ||
| $select = "(3958*3.1415926*sqrt((zipcode_searching.lat-zipcode_record.lat)*(zipcode_searching.lat-zipcode_record.lat) + cos(zipcode_searching.lat/57.29578)*cos(zipcode_record.lat/57.29578)*(zipcode_searching.lng-zipcode_record.lng)*(zipcode_searching.lng-zipcode_record.lng))/180) as distance_km"; | ||
| $join = "LEFT JOIN zipcodes as zipcode_searching ON zipcode_searching.zipcode = ".$zipcode." LEFT JOIN zipcodes as zipcode_record ON zipcode_record.zipcode = ".$zipcodeColumn; |
Comment on lines
+125
to
+128
| <?php if (PARSING_ENABLED && | ||
| is_countable($this->parsingStatus) && count($this->parsingStatus) && | ||
| $this->parsingStatus['parseUsed'] >= $this->parsingStatus['parseLimit'] && | ||
| $this->parsingStatus['parseLimit'] >= 0): ?> |
|
|
||
| <!-- CONTACT INFO --> | ||
| <?php if (count($this->departmentsRS) > 0): ?> | ||
| <?php if (is_array($this->departmentsRS) && count($this->departmentsRS) > 0): ?> |
| </tr> | ||
|
|
||
| <?php if (count($this->contactsRSWC) != 0): ?> | ||
| <?php if (is_array($this->contactsRSWC) && count($this->contactsRSWC) != 0): ?> |
|
|
||
| <?php /* The following are hidden by default */ ?> | ||
| <?php if (count($this->contactsRSWC) != count($this->contactsRS) && count($this->contactsRS) != 0) : ?> | ||
| <?php if ((is_array($this->contactsRSWC) && is_array($this->contactsRS)) && count($this->contactsRSWC) != count($this->contactsRS) && count($this->contactsRS) != 0) : ?> |
| </a> | ||
| <?php endif; ?> | ||
| <?php if (count($this->contactsRSWC) != count($this->contactsRS)) : ?> | ||
| <?php if (is_array($this->contactsRSWC) && is_array($this->contactsRS) && count($this->contactsRSWC) != count($this->contactsRS)) : ?> |
Comment on lines
+569
to
586
| if (LicenseUtility::isParsingEnabled()) | ||
| { | ||
| if (isset($res[$id='first_name']) && $res[$id] != '' && $firstName == '') $firstName = $res[$id]; | ||
| if (isset($res[$id='last_name']) && $res[$id] != '' && $lastName == '') $lastName = $res[$id]; | ||
| if (isset($res[$id='us_address']) && $res[$id] != '' && $address == '') $address = $res[$id]; | ||
| if (isset($res[$id='city']) && $res[$id] != '' && $city == '') $city = $res[$id]; | ||
| if (isset($res[$id='state']) && $res[$id] != '' && $state == '') $state = $res[$id]; | ||
| if (isset($res[$id='zip_code']) && $res[$id] != '' && $zip == '') $zip = $res[$id]; | ||
| if (isset($res[$id='email_address']) && $res[$id] != '' && $email == '') { $email = $res[$id]; $email2 = $res[$id]; $emailconfirm = $res[$id]; } | ||
| if (isset($res[$id='phone_number']) && $res[$id] != '' && $phone == '') $phone = $res[$id]; | ||
| if (isset($res[$id='skills']) && $res[$id] != '' && $keySkills == '') $keySkills = $res[$id]; | ||
| $pu = new ParseUtility(); | ||
| $fileName = isset($uploadFile) ? $uploadFile : ''; | ||
| $res = $pu->documentParse($fileName, strlen($resumeContents), '', $resumeContents); | ||
| if (is_array($res) && !empty($res)) | ||
| { | ||
| if (isset($res[$id='first_name']) && $res[$id] != '' && $firstName == '') $firstName = $res[$id]; | ||
| if (isset($res[$id='last_name']) && $res[$id] != '' && $lastName == '') $lastName = $res[$id]; | ||
| if (isset($res[$id='us_address']) && $res[$id] != '' && $address == '') $address = $res[$id]; | ||
| if (isset($res[$id='city']) && $res[$id] != '' && $city == '') $city = $res[$id]; | ||
| if (isset($res[$id='state']) && $res[$id] != '' && $state == '') $state = $res[$id]; | ||
| if (isset($res[$id='zip_code']) && $res[$id] != '' && $zip == '') $zip = $res[$id]; | ||
| if (isset($res[$id='email_address']) && $res[$id] != '' && $email == '') { $email = $res[$id]; $email2 = $res[$id]; $emailconfirm = $res[$id]; } | ||
| if (isset($res[$id='phone_number']) && $res[$id] != '' && $phone == '') $phone = $res[$id]; | ||
| if (isset($res[$id='skills']) && $res[$id] != '' && $keySkills == '') $keySkills = $res[$id]; | ||
| } | ||
| } |
Comment on lines
+678
to
+683
| // If parsing is enabled, add the image link for it | ||
| LicenseUtility::isParsingEnabled() ? | ||
| '<br /><div style="text-align: right;">' | ||
| . '<input type="button" value="Populate Fields ->" id="resumePopulate" onclick="resumeParse();" '.(strlen($resumeContents)?'':'disabled').' />' | ||
| : | ||
| '' |
Comment on lines
+469
to
+474
| } catch (Exception $e) { | ||
| // Mail not configured or failed - log error for debugging | ||
| $mailerStatus = false; | ||
|
|
||
| error_log('OpenCATS Mailer error (site=' . $this->_siteID . '): ' . $e->getMessage()); | ||
| } |
…uery() - Fix: change Earth radius from 3958 (miles) to 6371 (km) to match distance_km alias - Fix: cast zipcode to int to prevent SQL injection - Fix: validate zipcodeColumn against allowlist to prevent column injection
Contributor
Author
|
Thank you for the feedback and for pointing me to #792! I reviewed #792 and I can see it already addresses PHP 8.5 compatibility Some fixes in #809 that may complement #792 and are not immediately
I am happy to:
Whatever works best for the project. Thank you for maintaining OpenCATS! |
This was referenced Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tested on PHP 8.4.21 + MariaDB 10.11 on Debian 12