Skip to content

Add support for PHP 8.5#549

Merged
tenzap merged 7 commits into
kalkun-sms:develfrom
tenzap:feature-php8.5
Nov 21, 2025
Merged

Add support for PHP 8.5#549
tenzap merged 7 commits into
kalkun-sms:develfrom
tenzap:feature-php8.5

Conversation

@tenzap

@tenzap tenzap commented Oct 28, 2025

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Chores

    • Updated testing matrix and dependency patches to add PHP 8.4 and 8.5 support.
  • Bug Fixes

    • Adjusted resource cleanup to avoid issues on PHP 8.0+.
    • Revised test reflection handling for compatibility with PHP 8.1+.
  • Tests

    • Made test setup and related test code conditional on newer PHP versions for CI compatibility.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Oct 28, 2025

Copy link
Copy Markdown

Walkthrough

Adds PHP-version gated behavior: skip curl_close() when running PHP 8.0, skip ReflectionProperty::setAccessible() when running PHP 8.1+, updates composer patches to include PHP 8.5, extends CI matrix comments, and changes a microtime() cast from double to float.

Changes

Cohort / File(s) Summary
Conditional cURL cleanup
application/helpers/kalkun_helper.php, application/models/gateway/Connekt_model.php, application/models/gateway/Tmobilecz_model.php, application/models/gateway/Way2sms_model.php, application/plugins/sms_to_twitter/libraries/Epi.php, application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php
curl_close() calls are now executed only when PHP is not 8.0 (skip on PHP 8.0+)
Conditional ReflectionProperty access
tests/controllers/Pluginss_test.php, tests/testutils/DBSetup.php
ReflectionProperty::setAccessible(TRUE) calls are gated to run only on PHP versions below 8.1 (skip on PHP 8.1+)
Nonce seed cast change
application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php
Changed microtime() seed cast from double to float in generateNonce()
Phone number handling
application/models/Phonebook_model.php
country_code is explicitly cast to string before calling getCountryCodeForRegion() in convert_phonenumber()
CI/workflow & dependency patches
.github/workflows/codeigniter.yml, composer.json, composer-test_deps.json
CI matrix comments extended to mention PHP 8.4/8.5; composer.extra.patches updated to add econea/nusoap PHP 8.5 patch and adjust/remove some prior patches; test deps file adds kenjis/ci-phpunit-test PHP 8.5 patch entry

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant CURL as cURL handle
  participant PHPRuntime as PHP runtime check
  Note over Caller,CURL: HTTP request/response flow unchanged
  Caller->>CURL: perform request
  CURL-->>Caller: response
  Caller->>PHPRuntime: is_php('8.0')?
  alt PHP < 8.0
    PHPRuntime-->>CURL: close handle (curl_close)
  else PHP 8.0+
    PHPRuntime-->>Caller: skip curl_close
  end
Loading
sequenceDiagram
  participant TestSetup
  participant Reflection
  participant PHPRuntime as PHP runtime check
  TestSetup->>Reflection: attempt setAccessible(TRUE)
  TestSetup->>PHPRuntime: is_php('8.1')?
  alt PHP < 8.1
    PHPRuntime-->>Reflection: call setAccessible(TRUE)
  else PHP 8.1+
    PHPRuntime-->>TestSetup: skip setAccessible
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Cross-cutting PHP-version guards across several files; verify resource-management and test initialization implications.
  • Areas to focus:
    • Potential resource leaks or behavior changes from skipping curl_close() on PHP 8.0+
    • Test behavior when setAccessible() is skipped on PHP 8.1+
    • Composer patch entries and consistency with CI matrix

Possibly related PRs

Suggested reviewers

  • kingster

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'Add support for PHP 8.5' directly aligns with the main objective and changes throughout the PR, which collectively add PHP 8.5 compatibility across multiple files.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
application/models/Phonebook_model.php (1)

402-407: PHP 8+: fatal on property access of null; replace @-suppression with null-safe handling.

@ won’t save you from “Attempt to read property on null” (fatal in PHP 8+). Return null when no row.

 function group_id($group_name, $user_id)
 {
   $this->db->select('*');
   $this->db->from('pbk_groups');
   $this->db->where('Name', $group_name);
   $this->db->where('id_user', $user_id);
-  return @$this->db->get()->row()->ID;
+  $row = $this->db->get()->row();
+  return $row ? $row->ID : NULL;
 }
@@
 function group_name($group_id, $user_id)
 {
   $this->db->select('*');
   $this->db->from('pbk_groups');
   $this->db->where('ID', $group_id);
   $this->db->where('id_user', $user_id);
-  return @$this->db->get()->row()->Name;
+  $row = $this->db->get()->row();
+  return $row ? $row->Name : NULL;
 }

Also applies to: 421-426

🧹 Nitpick comments (1)
application/models/Phonebook_model.php (1)

508-510: Prefix-only replacement to avoid accidental mid-string changes; keep behavior stable.

Use a prefix check instead of global str_replace, and build international only when the local starts with '0'. Safe for PHP 8.5.

-        $number_local = str_replace($dial_code, '0', $param['number']);
-        $number_inter = $dial_code.substr($param['number'], 1);
+        $number = (string) $param['number'];
+        if (strpos($number, $dial_code) === 0) {
+            $number_local = '0'.ltrim(substr($number, strlen($dial_code)), '0');
+        } else {
+            $number_local = $number;
+        }
+        if (isset($number[0]) && $number[0] === '0') {
+            $number_inter = $dial_code.substr($number, 1);
+        } else {
+            $number_inter = $dial_code.$number;
+        }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 59a3dc8 and 00bf3e2.

⛔ Files ignored due to path filters (6)
  • composer.lock is excluded by !**/*.lock, !**/*.lock
  • patches/Econea_Nusoap/v0.9.17/0001-fix-deprecation-warning-on-PHP-8.4-with-xml_set_obje.patch is excluded by !**/*.patch
  • patches/Econea_Nusoap/v0.9.17/0002-check-if-SERVER_NAME-is-set.patch is excluded by !**/*.patch
  • patches/Econea_Nusoap/v0.9.18/0002-revert_fix_in_0.9.18.patch is excluded by !**/*.patch
  • patches/Econea_Nusoap/v0.9.20/support_php-8.5.patch is excluded by !**/*.patch
  • patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.5.patch is excluded by !**/*.patch
📒 Files selected for processing (11)
  • .github/workflows/codeigniter.yml (1 hunks)
  • application/helpers/kalkun_helper.php (1 hunks)
  • application/models/Phonebook_model.php (1 hunks)
  • application/models/gateway/Connekt_model.php (1 hunks)
  • application/models/gateway/Tmobilecz_model.php (1 hunks)
  • application/models/gateway/Way2sms_model.php (1 hunks)
  • application/plugins/sms_to_twitter/libraries/Epi.php (1 hunks)
  • application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (2 hunks)
  • composer.json (1 hunks)
  • tests/controllers/Pluginss_test.php (1 hunks)
  • tests/testutils/DBSetup.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
application/models/gateway/Way2sms_model.php (1)
application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (1)
  • curl (74-104)
application/models/gateway/Tmobilecz_model.php (1)
application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (1)
  • curl (74-104)
application/models/gateway/Connekt_model.php (1)
application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (1)
  • curl (74-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: test (ubuntu-latest, 7.1)
  • GitHub Check: test (ubuntu-latest, 7.3)
  • GitHub Check: test (ubuntu-latest, 5.6)
  • GitHub Check: test (ubuntu-latest, 7.4)
  • GitHub Check: test (ubuntu-latest, 7.2)
  • GitHub Check: test (ubuntu-latest, 8.1)
  • GitHub Check: test (ubuntu-latest, 8.0)
  • GitHub Check: test (ubuntu-latest, 8.4)
  • GitHub Check: test (ubuntu-latest, 7.0)
  • GitHub Check: test (ubuntu-latest, 8.3)
  • GitHub Check: test (ubuntu-latest, 8.2)
  • GitHub Check: check-code (ubuntu-latest, 8.2)
🔇 Additional comments (11)
.github/workflows/codeigniter.yml (1)

56-56: LGTM!

The comment update to include PHP 8.5 aligns with the PR's objective and provides documentation for the test matrix versions.

tests/testutils/DBSetup.php (1)

460-463: LGTM!

The PHP version guard correctly prevents calling setAccessible(TRUE) on PHP 8.1+, where private/protected properties became accessible by default through reflection.

application/models/gateway/Tmobilecz_model.php (1)

246-249: LGTM!

The PHP version guard correctly handles cURL resource cleanup. In PHP 8.0+, cURL handles are CurlHandle objects that auto-close when out of scope, so explicit curl_close() is unnecessary.

application/models/gateway/Way2sms_model.php (1)

157-160: LGTM!

Consistent with other gateway models, correctly gating curl_close() for PHP versions prior to 8.0.

composer.json (2)

59-59: LGTM!

Consolidating the econea/nusoap patches into a single PHP 8.5 support patch streamlines dependency management.


63-64: LGTM!

Adding PHP 8.5 support patch while maintaining the existing PHP 8.4 patch ensures compatibility across versions.

tests/controllers/Pluginss_test.php (1)

40-43: LGTM!

Consistently applies PHP 8.1+ compatibility guards for ReflectionProperty::setAccessible() across all static member resets. The pattern correctly skips the call on PHP 8.1+ where properties are accessible by default.

Also applies to: 47-50, 54-57, 61-63

application/plugins/sms_to_twitter/libraries/Epi.php (1)

157-160: LGTM!

The cURL handle cleanup is correctly positioned after all cURL operations (lines 151-156), and properly gated for PHP versions prior to 8.0.

application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (2)

159-159: LGTM! Cosmetic cast change.

The change from (double) to (float) is purely stylistic—both are aliases for the same floating-point type in PHP. The float convention is more common.


99-102: The review comment is incorrect.

The is_php('8.0') function is a CodeIgniter helper that's globally available throughout the application. This is confirmed by its consistent usage across the entire codebase in production models, helpers, plugins, and tests—all without any guards or function_exists checks. Other third-party libraries integrated in this codebase (like sms_to_twitter/libraries/Epi.php) use the identical pattern without issue. The curl_close conditional at lines 99-102 is correct and will execute safely.

Likely an incorrect or invalid review comment.

application/models/gateway/Connekt_model.php (1)

83-86: LGTM! Correct PHP 8.0+ curl handle cleanup.

The conditional curl_close pattern is appropriate. In PHP 8.0+, curl handles are CurlHandle objects that automatically clean up when they go out of scope, making explicit closure unnecessary. This pattern is consistent with similar changes across the PR.

Comment on lines +34 to 40
if ( ! is_php('8.0'))
{
curl_close ($request);
}

$httpcode = curl_getinfo($request, CURLINFO_HTTP_CODE);
if ($httpcode !== 200)

@coderabbitai coderabbitai Bot Oct 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: curl_getinfo called after curl_close on PHP < 8.0.

Line 39 calls curl_getinfo($request, CURLINFO_HTTP_CODE) after the conditional curl_close() at line 36. On PHP versions prior to 8.0, the handle is closed before curl_getinfo() is invoked, which will cause the function to fail or return invalid data.

Apply this diff to fix the order:

 	$response = curl_exec($request);
+	$httpcode = curl_getinfo($request, CURLINFO_HTTP_CODE);
 	if ( ! is_php('8.0'))
 	{
 		curl_close ($request);
 	}
 
-	$httpcode = curl_getinfo($request, CURLINFO_HTTP_CODE);
 	if ($httpcode !== 200)
 	{

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In application/helpers/kalkun_helper.php around lines 34-40, curl_getinfo is
being called after curl_close for PHP < 8.0 which can fail; move the call to
$httpcode = curl_getinfo($request, CURLINFO_HTTP_CODE) to occur before the
conditional curl_close(...) block so the HTTP code is retrieved while the handle
is still valid, then perform curl_close($request) inside the existing
is_php('8.0') check.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one @tenzap

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@tenzap tenzap Nov 21, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm a bit dubious. This was already like that before and this pr for php 8.5 doesn't add a regression here. Is this really required? Btw is DND in India still working today as 15 years ago? It is likely I added that regression at sometime because DND doesn't work anymore iirc.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point :)

Yes DND has evolved, and there is much larger registry for DND. These would need to be updated for such use

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this for now and make the change suggested here appart later (through a pr to check in ci, and then merge if it passes).

Comment on lines +507 to 510
$dial_code = '+'.$phoneNumberUtil->getCountryCodeForRegion(strval($country_code));
$number_local = str_replace($dial_code, '0', $param['number']);
$number_inter = $dial_code.substr($param['number'], 1);
return array($param['number'], $number_local, $number_inter);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Good cast; add normalization and guard for invalid/empty region (prevents '+0').

Casting to string is correct for PHP 8.x. Add uppercasing and handle empty/unknown region to avoid producing '+0' and bad local/international variants.

Apply minimally defensive changes:

-        $dial_code = '+'.$phoneNumberUtil->getCountryCodeForRegion(strval($country_code));
+        $region = strtoupper((string) $country_code);
+        $cc = $phoneNumberUtil->getCountryCodeForRegion($region);
+        if ($cc === 0) {
+            log_message('warning', 'Unknown/empty country_code for user '.$param['id_user'].' in convert_phonenumber()');
+            // keep current behavior but avoid '+0'
+            return array($param['number'], $param['number'], $param['number']);
+        }
+        $dial_code = '+'.$cc;

Verify what is stored in settings to ensure we're passing an ISO 3166-1 alpha-2 region (e.g., 'ID'), not a calling code (e.g., '62'):


🏁 Script executed:

#!/bin/bash
# Inspect usages and likely values of `country_code` across the repo
rg -n -C3 '\bcountry_code\b' --hidden -g '!vendor/**'
rg -n -C3 'get_setting\s*\(.*\)\s*->row\(\s*([\'"])country_code\1\)' --hidden -g '!vendor/**'
# Check if code elsewhere treats it as a region vs calling code
rg -n -C3 'getCountryCodeForRegion\(' --hidden -g '!vendor/**'

Length of output: 22867


Add uppercasing and guard against invalid regions to prevent '+0' dial codes.

The cast to string is correct for PHP 8.x. However, region codes must be uppercase, and getCountryCodeForRegion() returns 0 for invalid regions, which would produce a "+0" dial code. Add normalization and validation:

-        $dial_code = '+'.$phoneNumberUtil->getCountryCodeForRegion(strval($country_code));
+        $region = strtoupper((string) $country_code);
+        $cc = $phoneNumberUtil->getCountryCodeForRegion($region);
+        if ($cc === 0) {
+            log_message('warning', 'Unknown/empty country_code for user '.$param['id_user'].' in convert_phonenumber()');
+            // keep current behavior but avoid '+0'
+            return array($param['number'], $param['number'], $param['number']);
+        }
+        $dial_code = '+'.$cc;
🤖 Prompt for AI Agents
In application/models/Phonebook_model.php around lines 507 to 510, the region
passed to getCountryCodeForRegion must be uppercased and its return value
validated because getCountryCodeForRegion returns 0 for invalid regions (which
would produce "+0"). Normalize the region with
strtoupper(strval($country_code)), call getCountryCodeForRegion on that, check
the result is non-zero before building $dial_code (and handle the invalid case —
e.g., skip replacement/formatting or return original number with a
logged/handled error), then proceed to compute $number_local and $number_inter
only when a valid dial code exists.

@tenzap

tenzap commented Nov 20, 2025

Copy link
Copy Markdown
Collaborator Author

@kingster ?

@kingster

Copy link
Copy Markdown
Collaborator

There are few critical issues that has been identified by coderabbit, maybe we should fix it?

@tenzap

tenzap commented Nov 21, 2025

Copy link
Copy Markdown
Collaborator Author

Which precisely do you think of? I don't think they are related to PHP 8.5 support, so should be addressed later in another pr/issue when one has time and motivation for it.

…ce 8.5

Method ReflectionProperty::setAccessible() is deprecated since 8.5, as it has no effect on line 40 in file /home/runner/work/Kalkun/Kalkun/tests/controllers/Pluginss_test.php

Method ReflectionProperty::setAccessible() is deprecated since 8.5, as it has no effect on line 460 in file /home/runner/work/Kalkun/Kalkun/tests/testutils/DBSetup.php
DeprecatedException: Function curl_close() is deprecated since 8.5, as it has no effect since PHP 8.0
…rgument #1 () must be of type string, null given

TypeError: libphonenumber\PhoneNumberUtil::getCountryCodeForRegion(): Argument #1 () must be of type string, null given, called in /home/runner/work/Kalkun/Kalkun/application/models/Phonebook_model.php on line 507

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00bf3e2 and 4e16586.

⛔ Files ignored due to path filters (6)
  • composer.lock is excluded by !**/*.lock, !**/*.lock
  • patches/Econea_Nusoap/v0.9.17/0001-fix-deprecation-warning-on-PHP-8.4-with-xml_set_obje.patch is excluded by !**/*.patch
  • patches/Econea_Nusoap/v0.9.17/0002-check-if-SERVER_NAME-is-set.patch is excluded by !**/*.patch
  • patches/Econea_Nusoap/v0.9.18/0002-revert_fix_in_0.9.18.patch is excluded by !**/*.patch
  • patches/Econea_Nusoap/v0.9.20/support_php-8.5.patch is excluded by !**/*.patch
  • patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.5.patch is excluded by !**/*.patch
📒 Files selected for processing (11)
  • .github/workflows/codeigniter.yml (1 hunks)
  • application/helpers/kalkun_helper.php (1 hunks)
  • application/models/Phonebook_model.php (1 hunks)
  • application/models/gateway/Connekt_model.php (1 hunks)
  • application/models/gateway/Tmobilecz_model.php (1 hunks)
  • application/models/gateway/Way2sms_model.php (1 hunks)
  • application/plugins/sms_to_twitter/libraries/Epi.php (1 hunks)
  • application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (2 hunks)
  • composer.json (1 hunks)
  • tests/controllers/Pluginss_test.php (1 hunks)
  • tests/testutils/DBSetup.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • application/helpers/kalkun_helper.php
  • tests/testutils/DBSetup.php
  • application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php
  • application/plugins/sms_to_twitter/libraries/Epi.php
  • application/models/Phonebook_model.php
  • .github/workflows/codeigniter.yml
  • tests/controllers/Pluginss_test.php
🧰 Additional context used
🧬 Code graph analysis (3)
application/models/gateway/Way2sms_model.php (2)
application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (1)
  • curl (74-104)
scripts/cURL/Kalkun_API.php (3)
  • send_sms (102-125)
  • Kalkun_API (9-141)
  • run (48-56)
application/models/gateway/Tmobilecz_model.php (2)
application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (1)
  • curl (74-104)
scripts/cURL/Kalkun_API.php (2)
  • Kalkun_API (9-141)
  • finish (58-67)
application/models/gateway/Connekt_model.php (1)
application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php (1)
  • curl (74-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: test (ubuntu-latest, 5.6)
  • GitHub Check: test (ubuntu-latest, 7.3)
  • GitHub Check: test (ubuntu-latest, 7.4)
  • GitHub Check: test (ubuntu-latest, 7.2)
  • GitHub Check: test (ubuntu-latest, 8.5)
  • GitHub Check: test (ubuntu-latest, 7.1)
  • GitHub Check: test (ubuntu-latest, 8.1)
  • GitHub Check: test (ubuntu-latest, 7.0)
  • GitHub Check: test (ubuntu-latest, 8.0)
  • GitHub Check: test (ubuntu-latest, 8.4)
  • GitHub Check: test (ubuntu-latest, 8.2)
  • GitHub Check: test (ubuntu-latest, 8.3)
  • GitHub Check: check-code (ubuntu-latest, 8.2)
🔇 Additional comments (4)
composer.json (2)

53-53: Verify that consolidating econea/nusoap patches maintains PHP 8.4 compatibility.

The new "Add support for PHP 8.5" patch replaces three previous patches that addressed PHP 8.4 deprecations, $_SERVER checks, and regression fixes. Ensure that the new patch includes coverage for these PHP 8.4 issues, or that they are no longer needed.


53-58: Verify patch files exist and match dependency versions.

econea/nusoap officially supports PHP 5.4 - 8.2, but the changes reference a patch claiming to add PHP 8.5 support. Verify that:

  1. All patch files exist at their specified paths (patches/Econea_Nusoap/v0.9.20/support_php-8.5.patch, patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.4.patch, patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.5.patch)
  2. The patch file versions match the locked dependency versions in composer.lock
  3. The patches are compatible with composer install and composer update operations
application/models/gateway/Way2sms_model.php (1)

157-160: Same concern about skipping curl_close() on PHP 8.0+.

This change follows the same pattern as in Tmobilecz_model.php. Please refer to the verification request in that file regarding whether skipping curl_close() is necessary for PHP 8.0+ compatibility.

application/models/gateway/Connekt_model.php (1)

83-86: Same curl_close() pattern applied consistently.

This change follows the same pattern as in Tmobilecz_model.php and Way2sms_model.php. The consistency is good, but please refer to the verification request in Tmobilecz_model.php regarding whether this change is necessary for PHP 8.0+ compatibility.

Comment on lines +246 to +249
if ( ! is_php('8.0'))
{
curl_close ($curl);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Verify the necessity of skipping curl_close() on PHP 8.0+.

While the change follows the pattern established in application/plugins/sms_to_xmpp/libraries/abhinavsingh-JAXL-5829c3b/core/jaxl.util.php, it's unclear whether skipping curl_close() is actually necessary for PHP 8.0+ compatibility. In PHP 8.0+, curl handles became CurlHandle objects with automatic cleanup, but curl_close() should still be safe to call and is not deprecated.

Please search for documentation to confirm whether there are any issues with calling curl_close() explicitly in PHP 8.0+ that would justify skipping it:

Additionally, note that scripts/cURL/Kalkun_API.php line 60 still calls curl_close($ch) unconditionally. If skipping curl_close() on PHP 8.0+ is necessary, that file should also be updated for consistency.


🌐 Web query:

Does curl_close() cause issues or warnings in PHP 8.0 and later versions?

💡 Result:

Short answer: No serious issues in PHP 8.0–8.4, but it became a no‑op; starting with PHP 8.5 it is deprecated and will raise deprecation warnings.

Details:

  • In PHP 8.0 cURL handles were converted from resources to CurlHandle objects, so curl_close() no longer actually closes anything (handles are freed by object destruction/GC). Calling it is harmless in 8.0–8.4. [1][2]
  • The function was marked deprecated in the PHP 8.5 deprecations RFC/manual and will emit deprecation warnings in PHP 8.5+. Codebases have been updated to avoid calling these now no‑op functions. [1][3]

If you need compatibility: either unset the handle (unset($ch)) or stop calling curl_close() on PHP 8.5+ (guard the call by PHP version or by checking the handle type). [2][3]

Sources:
[1] PHP manual — curl_close.
[2] php.watch — CurlHandle / migration notes.
[3] PHP 8.5 deprecations / changelogs (no‑op function deprecations).


Update curl_close() guard to only skip on PHP 8.5+, not PHP 8.0+.

The current implementation is overly conservative. In PHP 8.0–8.4, curl_close() is harmless (it became a no-op when cURL handles became objects), but the code skips it for all PHP 8.0+ versions. The deprecation warning only applies to PHP 8.5+. Correct the condition to:

if ( ! is_php('8.5'))
{
    curl_close($curl);
}

Or alternatively, remove the guard entirely for PHP 8.0–8.4 compatibility and add it only when PHP 8.5+ support becomes critical. Additionally, scripts/cURL/Kalkun_API.php line 60 still calls curl_close() unconditionally—update it for consistency if this pattern is adopted.

🤖 Prompt for AI Agents
In application/models/gateway/Tmobilecz_model.php around lines 246 to 249, the
guard that skips curl_close() for PHP 8.0+ is too broad; change it so
curl_close() is skipped only for PHP 8.5+ (i.e., update the version check to
8.5) or remove the guard entirely and call curl_close() normally for PHP
8.0–8.4; also update scripts/cURL/Kalkun_API.php line 60 to apply the same
conditional guard (or remove its guard) so both places are consistent.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e16586 and a373055.

⛔ Files ignored due to path filters (4)
  • composer-test_deps.lock is excluded by !**/*.lock, !**/*.lock
  • composer.lock is excluded by !**/*.lock, !**/*.lock
  • patches/Econea_Nusoap/v0.9.20/support_php-8.5.patch is excluded by !**/*.patch
  • patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.5.patch is excluded by !**/*.patch
📒 Files selected for processing (3)
  • .github/workflows/codeigniter.yml (1 hunks)
  • composer-test_deps.json (1 hunks)
  • composer.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/codeigniter.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: test (ubuntu-latest, 5.6)
  • GitHub Check: test (ubuntu-latest, 7.3)
  • GitHub Check: test (ubuntu-latest, 7.4)
  • GitHub Check: test (ubuntu-latest, 7.2)
  • GitHub Check: test (ubuntu-latest, 7.1)
  • GitHub Check: test (ubuntu-latest, 8.5)
  • GitHub Check: test (ubuntu-latest, 8.3)
  • GitHub Check: test (ubuntu-latest, 7.0)
  • GitHub Check: test (ubuntu-latest, 8.4)
  • GitHub Check: test (ubuntu-latest, 8.0)
  • GitHub Check: test (ubuntu-latest, 8.2)
  • GitHub Check: test (ubuntu-latest, 8.1)
  • GitHub Check: check-code (ubuntu-latest, 8.2)
🔇 Additional comments (1)
composer-test_deps.json (1)

12-13: Additive patch extension is sound.

The approach of adding the PHP 8.5 patch alongside existing entries for 8.2 and 8.4 preserves backward compatibility. This pattern is consistent with established practices in this file.

Comment thread composer.json
@tenzap tenzap merged commit ddd4fe4 into kalkun-sms:devel Nov 21, 2025
16 checks passed
@tenzap tenzap deleted the feature-php8.5 branch November 21, 2025 19:39
tenzap added a commit to tenzap/Kalkun that referenced this pull request Nov 21, 2025
tenzap added a commit to tenzap/Kalkun that referenced this pull request Nov 21, 2025
tenzap added a commit to tenzap/Kalkun that referenced this pull request Nov 21, 2025
tenzap added a commit that referenced this pull request Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants