From a96580bf917f77eec6b27442bf66525cc3e307ea Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Wed, 14 Oct 2026 04:16:13 -0400 Subject: [PATCH 1/3] Fix the correctness findings from the Qodana scan image.php called imagerotate() with four arguments, a fatal error on PHP 8 when an angle is requested. The LDAP and Active Directory auth classes used the ldap_connect($host, $port) form that PHP 8.3 deprecates; they now build one ldap:// URI and keep a scheme the admin already configured. The cache engine base class throws LogicException for a missing read() or write() instead of trigger_error(E_USER_ERROR), deprecated in PHP 8.4. xoops_aw_encode() and xoops_aw_decode() are array_walk() callbacks but took their element by value, so they were no-ops; they now take it by reference, and write.php serializes the walked array (quoted through the database handler) instead of array_walk()'s boolean. xoops_getUrlDomain() checks the host key only when parse_url() returned an array. --- docs/changelog.270.txt | 5 +++++ htdocs/class/auth/auth_ads.php | 6 +++++- htdocs/class/auth/auth_ldap.php | 6 +++++- htdocs/class/cache/xoopscache.php | 4 ++-- htdocs/class/model/write.php | 3 ++- htdocs/image.php | 2 +- htdocs/include/functions.encoding.php | 16 ++++++++++------ htdocs/include/functions.php | 2 +- tests/unit/htdocs/class/LegacyClassLoadTest.php | 16 +++++----------- 9 files changed, 36 insertions(+), 24 deletions(-) diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index c226e552b..a700d6d8f 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -4,6 +4,11 @@ XOOPS 2.7.x Changelog (Language changes: see: /docs/lang_diff.txt) 2.7.4 NOT RELEASED =================================== Bug fixes: +- image.php no longer crashes on PHP 8 when an angle is requested: imagerotate() is called with three arguments, the fourth was removed in PHP 8.0 (image.php) +- LDAP and Active Directory logins on PHP 8.3+ no longer emit a deprecation: ldap_connect() receives one ldap:// URI built from the configured server and port, and a server already written with an ldap:// or ldaps:// scheme is used as is (class/auth/auth_ldap.php, class/auth/auth_ads.php) +- a cache engine without read() or write() throws LogicException instead of trigger_error(E_USER_ERROR), which PHP 8.4 deprecates (class/cache/xoopscache.php) +- xoops_aw_encode() and xoops_aw_decode() take their element by reference, so the array_walk() calls that encode and decode XOBJ_DTYPE_UNICODE_ARRAY values act on the array; write.php serializes the walked array instead of array_walk()'s return value and quotes it through the database handler (include/functions.encoding.php, class/model/write.php, kernel/object.php) +- xoops_getUrlDomain() checks the host key only when parse_url() returned an array (include/functions.php) - templates can use |sprintf without a Smarty 4 deprecation notice: XoopsTpl registers sprintf as a modifier, as it already does for strstr, trim and file_exists (class/template.php) (#264) Libraries and assets: diff --git a/htdocs/class/auth/auth_ads.php b/htdocs/class/auth/auth_ads.php index 923fef8bd..ad1682d77 100644 --- a/htdocs/class/auth/auth_ads.php +++ b/htdocs/class/auth/auth_ads.php @@ -66,7 +66,11 @@ public function authenticate($uname, $pwd = null) return $authenticated; } - $this->_ds = ldap_connect($this->ldap_server, $this->ldap_port); + // PHP 8.3 deprecated the ($host, $port) form; pass one URI and keep a scheme the admin configured. + $ldapUri = preg_match('#^ldaps?://#i', (string) $this->ldap_server) + ? $this->ldap_server + : 'ldap://' . $this->ldap_server . ':' . $this->ldap_port; + $this->_ds = ldap_connect($ldapUri); if ($this->_ds) { ldap_set_option($this->_ds, LDAP_OPT_PROTOCOL_VERSION, $this->ldap_version); ldap_set_option($this->_ds, LDAP_OPT_REFERRALS, 0); diff --git a/htdocs/class/auth/auth_ldap.php b/htdocs/class/auth/auth_ldap.php index cb7e96ea7..e00a9c924 100644 --- a/htdocs/class/auth/auth_ldap.php +++ b/htdocs/class/auth/auth_ldap.php @@ -235,7 +235,11 @@ public function authenticate($uname, $pwd = null) return $authenticated; } - $this->_ds = ldap_connect($this->ldap_server, $this->ldap_port); + // PHP 8.3 deprecated the ($host, $port) form; pass one URI and keep a scheme the admin configured. + $ldapUri = preg_match('#^ldaps?://#i', (string) $this->ldap_server) + ? $this->ldap_server + : 'ldap://' . $this->ldap_server . ':' . $this->ldap_port; + $this->_ds = ldap_connect($ldapUri); if ($this->_ds) { ldap_set_option($this->_ds, LDAP_OPT_PROTOCOL_VERSION, $this->ldap_version); if ($this->ldap_use_TLS) { // We use TLS secure connection diff --git a/htdocs/class/cache/xoopscache.php b/htdocs/class/cache/xoopscache.php index 635c2db22..fb903453b 100644 --- a/htdocs/class/cache/xoopscache.php +++ b/htdocs/class/cache/xoopscache.php @@ -705,7 +705,7 @@ public function gc() */ public function write($key, $value, $duration = null) { - trigger_error(sprintf('Method write() not implemented in %s', get_class($this)), E_USER_ERROR); + throw new LogicException(sprintf('Method write() not implemented in %s', get_class($this))); } /** @@ -717,7 +717,7 @@ public function write($key, $value, $duration = null) */ public function read($key) { - trigger_error(sprintf('Method read() not implemented in %s', get_class($this)), E_USER_ERROR); + throw new LogicException(sprintf('Method read() not implemented in %s', get_class($this))); } /** diff --git a/htdocs/class/model/write.php b/htdocs/class/model/write.php index 7ebe3fe8e..95cc18df1 100644 --- a/htdocs/class/model/write.php +++ b/htdocs/class/model/write.php @@ -197,7 +197,8 @@ public function cleanVars($object) $cleanv[$key] = str_replace('\\"', '"', addslashes($cleanv[$key])); } // TODO: Not encoding safe, should try base64_encode -- phppp - $cleanv = "'" . serialize(array_walk($cleanv, 'xoops_aw_encode')) . "'"; + array_walk($cleanv, 'xoops_aw_encode'); + $cleanv = $this->handler->db->quote(serialize($cleanv)); break; case XOBJ_DTYPE_ARRAY: diff --git a/htdocs/image.php b/htdocs/image.php index ec2816ac4..ed160dfea 100644 --- a/htdocs/image.php +++ b/htdocs/image.php @@ -688,7 +688,7 @@ function imageFilenameCheck($imageUrl) // Imagerotate if (ENABLE_IMAGEROTATE && !empty($angle)) { - $destination_image = imagerotate($destination_image, $angle, $background, 0); + $destination_image = imagerotate($destination_image, $angle, $background); } if ($do_sharpen) { diff --git a/htdocs/include/functions.encoding.php b/htdocs/include/functions.encoding.php index 062a0a53d..d997b81e2 100644 --- a/htdocs/include/functions.encoding.php +++ b/htdocs/include/functions.encoding.php @@ -128,12 +128,14 @@ function xoops_convert_decode($data, $store_method = 'urlcode') /** * xoops_aw_encode() * - * @param mixed $value value of array + * array_walk() callback: encodes the array element in place. + * + * @param mixed $value value of array, passed by reference * @param mixed $key key of array * @param string $store_method - * @return boolean|string + * @return void */ -function xoops_aw_encode($value, $key, $store_method = 'urlcode') +function xoops_aw_encode(&$value, $key, $store_method = 'urlcode') { $value = xoops_convert_encode($value, $store_method); } @@ -141,12 +143,14 @@ function xoops_aw_encode($value, $key, $store_method = 'urlcode') /** * xoops_aw_decode() * - * @param mixed $value value of array + * array_walk() callback: decodes the array element in place. + * + * @param mixed $value value of array, passed by reference * @param mixed $key key of array * @param string $store_method - * @return boolean|string + * @return void */ -function xoops_aw_decode($value, $key, $store_method = 'urlcode') +function xoops_aw_decode(&$value, $key, $store_method = 'urlcode') { $value = xoops_convert_decode($value, $store_method); } diff --git a/htdocs/include/functions.php b/htdocs/include/functions.php index 64cf0a7f8..f06a323f3 100644 --- a/htdocs/include/functions.php +++ b/htdocs/include/functions.php @@ -1381,7 +1381,7 @@ function xoops_getUrlDomain($url) $domain = ''; $_URL = parse_url($url); - if (!empty($_URL) || !empty($_URL['host'])) { + if (!empty($_URL) && !empty($_URL['host'])) { $domain = $_URL['host']; } diff --git a/tests/unit/htdocs/class/LegacyClassLoadTest.php b/tests/unit/htdocs/class/LegacyClassLoadTest.php index 45a37ec20..c44943202 100644 --- a/tests/unit/htdocs/class/LegacyClassLoadTest.php +++ b/tests/unit/htdocs/class/LegacyClassLoadTest.php @@ -160,18 +160,12 @@ public function delete($key) } }; $messages = []; - set_error_handler(static function (int $no, string $msg) use (&$messages): bool { - if ($no === E_USER_ERROR) { - $messages[] = $msg; + foreach ([static fn () => $engine->write('k', 'v'), static fn () => $engine->read('k')] as $call) { + try { + $call(); + } catch (\LogicException $e) { + $messages[] = $e->getMessage(); } - - return true; - }); - try { - $engine->write('k', 'v'); - $engine->read('k'); - } finally { - restore_error_handler(); } self::assertSame([ From 2ba655cd516d2d444fefec90b1a60037716a9fae Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Wed, 14 Oct 2026 04:31:13 -0400 Subject: [PATCH 2/3] Address review on the Qodana fixes XoopsObject::getVar() and setVar() had the same array_walk() misuse as write.php: getVar() returned true instead of the decoded unicode array and setVar() serialized a boolean. The LDAP URI builder brackets a bare IPv6 address, and STARTTLS is skipped on an ldaps:// URI, which is already encrypted. The changelog names PHP 8.3 as the version that removed the fourth imagerotate() argument; 8.0 to 8.2 still accept it. --- docs/changelog.270.txt | 6 +++--- htdocs/class/auth/auth_ads.php | 12 ++++++++---- htdocs/class/auth/auth_ldap.php | 12 ++++++++---- htdocs/kernel/object.php | 5 +++-- 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index a700d6d8f..9aec456d6 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -4,10 +4,10 @@ XOOPS 2.7.x Changelog (Language changes: see: /docs/lang_diff.txt) 2.7.4 NOT RELEASED =================================== Bug fixes: -- image.php no longer crashes on PHP 8 when an angle is requested: imagerotate() is called with three arguments, the fourth was removed in PHP 8.0 (image.php) -- LDAP and Active Directory logins on PHP 8.3+ no longer emit a deprecation: ldap_connect() receives one ldap:// URI built from the configured server and port, and a server already written with an ldap:// or ldaps:// scheme is used as is (class/auth/auth_ldap.php, class/auth/auth_ads.php) +- image.php no longer crashes on PHP 8.3+ when an angle is requested: imagerotate() is called with three arguments, the fourth was removed in PHP 8.3 (image.php) +- LDAP and Active Directory logins on PHP 8.3+ no longer emit a deprecation: ldap_connect() receives one ldap:// URI built from the configured server and port, a bare IPv6 address is bracketed, a server already written with an ldap:// or ldaps:// scheme is used as is, and STARTTLS is not attempted on an ldaps:// connection (class/auth/auth_ldap.php, class/auth/auth_ads.php) - a cache engine without read() or write() throws LogicException instead of trigger_error(E_USER_ERROR), which PHP 8.4 deprecates (class/cache/xoopscache.php) -- xoops_aw_encode() and xoops_aw_decode() take their element by reference, so the array_walk() calls that encode and decode XOBJ_DTYPE_UNICODE_ARRAY values act on the array; write.php serializes the walked array instead of array_walk()'s return value and quotes it through the database handler (include/functions.encoding.php, class/model/write.php, kernel/object.php) +- xoops_aw_encode() and xoops_aw_decode() take their element by reference, so the array_walk() calls that encode and decode XOBJ_DTYPE_UNICODE_ARRAY values act on the array; write.php and XoopsObject::setVar() serialize the walked array instead of array_walk()'s boolean, and XoopsObject::getVar() returns the decoded array instead of true (include/functions.encoding.php, class/model/write.php, kernel/object.php) - xoops_getUrlDomain() checks the host key only when parse_url() returned an array (include/functions.php) - templates can use |sprintf without a Smarty 4 deprecation notice: XoopsTpl registers sprintf as a modifier, as it already does for strstr, trim and file_exists (class/template.php) (#264) diff --git a/htdocs/class/auth/auth_ads.php b/htdocs/class/auth/auth_ads.php index ad1682d77..c7682bb9c 100644 --- a/htdocs/class/auth/auth_ads.php +++ b/htdocs/class/auth/auth_ads.php @@ -67,14 +67,18 @@ public function authenticate($uname, $pwd = null) return $authenticated; } // PHP 8.3 deprecated the ($host, $port) form; pass one URI and keep a scheme the admin configured. - $ldapUri = preg_match('#^ldaps?://#i', (string) $this->ldap_server) - ? $this->ldap_server - : 'ldap://' . $this->ldap_server . ':' . $this->ldap_port; + $ldapServer = (string) $this->ldap_server; + if (filter_var($ldapServer, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false) { + $ldapServer = '[' . $ldapServer . ']'; + } + $ldapUri = preg_match('#^ldaps?://#i', $ldapServer) + ? $ldapServer + : 'ldap://' . $ldapServer . ':' . $this->ldap_port; $this->_ds = ldap_connect($ldapUri); if ($this->_ds) { ldap_set_option($this->_ds, LDAP_OPT_PROTOCOL_VERSION, $this->ldap_version); ldap_set_option($this->_ds, LDAP_OPT_REFERRALS, 0); - if ($this->ldap_use_TLS) { // We use TLS secure connection + if ($this->ldap_use_TLS && stripos($ldapUri, 'ldaps://') !== 0) { // STARTTLS; an ldaps:// URI is already encrypted if (!ldap_start_tls($this->_ds)) { $this->setErrors(0, _AUTH_LDAP_START_TLS_FAILED); // TLS was required: never look up or bind over the plain connection. diff --git a/htdocs/class/auth/auth_ldap.php b/htdocs/class/auth/auth_ldap.php index e00a9c924..ed6579ef6 100644 --- a/htdocs/class/auth/auth_ldap.php +++ b/htdocs/class/auth/auth_ldap.php @@ -236,13 +236,17 @@ public function authenticate($uname, $pwd = null) return $authenticated; } // PHP 8.3 deprecated the ($host, $port) form; pass one URI and keep a scheme the admin configured. - $ldapUri = preg_match('#^ldaps?://#i', (string) $this->ldap_server) - ? $this->ldap_server - : 'ldap://' . $this->ldap_server . ':' . $this->ldap_port; + $ldapServer = (string) $this->ldap_server; + if (filter_var($ldapServer, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false) { + $ldapServer = '[' . $ldapServer . ']'; + } + $ldapUri = preg_match('#^ldaps?://#i', $ldapServer) + ? $ldapServer + : 'ldap://' . $ldapServer . ':' . $this->ldap_port; $this->_ds = ldap_connect($ldapUri); if ($this->_ds) { ldap_set_option($this->_ds, LDAP_OPT_PROTOCOL_VERSION, $this->ldap_version); - if ($this->ldap_use_TLS) { // We use TLS secure connection + if ($this->ldap_use_TLS && stripos($ldapUri, 'ldaps://') !== 0) { // STARTTLS; an ldaps:// URI is already encrypted if (!ldap_start_tls($this->_ds)) { $this->setErrors(0, _AUTH_LDAP_START_TLS_FAILED); // TLS was required: never look up or bind over the plain connection. diff --git a/htdocs/kernel/object.php b/htdocs/kernel/object.php index 336589ace..5e16b347b 100644 --- a/htdocs/kernel/object.php +++ b/htdocs/kernel/object.php @@ -550,7 +550,7 @@ public function getVar($key, ?string $format = null) } $ret = is_array($ret) ? $ret : []; if (is_array($ret)) { - $ret = array_walk($ret, 'xoops_aw_decode'); + array_walk($ret, 'xoops_aw_decode'); } } @@ -885,7 +885,8 @@ public function cleanVars() $cleanv = xoops_convert_encode($cleanv); break; case /** @scrutinizer ignore-deprecated */ XOBJ_DTYPE_UNICODE_ARRAY: - $cleanv = serialize(array_walk($cleanv, 'xoops_aw_encode')); + array_walk($cleanv, 'xoops_aw_encode'); + $cleanv = serialize($cleanv); break; default: break; From 22037c65fd983f8c18da95eadd367209e9e0e4a2 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Wed, 14 Oct 2026 04:53:05 -0400 Subject: [PATCH 3/3] Document the integer id XoopsUser::__construct() accepts The docblock said array|null, but every caller passes a user id; Scrutinizer flagged each of them once getVar() stopped returning a boolean for unicode arrays. --- htdocs/kernel/user.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/kernel/user.php b/htdocs/kernel/user.php index 47a55cbf8..5d90b64fd 100644 --- a/htdocs/kernel/user.php +++ b/htdocs/kernel/user.php @@ -91,7 +91,7 @@ class XoopsUser extends XoopsObject /** * constructor - * @param array|null $id ID of the user to be loaded from the database. + * @param int|array|null $id ID of the user to be loaded from the database, or an array of values */ public function __construct($id = null) {