Project Beehive ForumNull values from query functions.

 

Press Ctrl+Enter to quickly submit your post
Quick Reply  
 
 
  
 From:  Peter (BOUGHTONP)  
 To:  Matt     
42762.1 
Hey Matt,

I'm getting errors with PHP 7.4.15 and MariaDB 10.5.9 on Debian.

For example, function thread_has_attachments in include/thread.inc.php - line 1365 should return false but doesn't.
Same situation for stats_get_most_popular_birthday in include/stats.inc.php line 1499

In both cases, the queries correctly return no values, but the if condition is failing:

if (!($result = $db->query($sql))) return false;

Is this a PHP change, a DB driver issue, or something else?

0/0
 Reply   Quote More 

 From:  Peter (BOUGHTONP)  
 To:  Peter (BOUGHTONP)     
42762.2 In reply to 42762.1 
Seems to work if I add "->num_rows", i.e:
if (!($result = $db->query($sql))->num_rows) return false;

The docs suggest this works in PHP 5, so a mass search-replace won't break backwards compatibility?

https://www.php.net/manual/en/mysqli-result.num-rows.php

0/0
 Reply   Quote More 

 From:  Peter (BOUGHTONP)  
 To:  Peter (BOUGHTONP)     
42762.3 In reply to 42762.2 
> Seems to work if I add "->num_rows", i.e:

Except for UPDATE queries which return booleans instead of objects, such as in the user_logon function.

Stupid PHP.

Also, checking a few of the 391 matches, some of them already have additional num_rows checks on separate rows.

Not sure if it'll be easier to click every link and fix them as they crop up, but I haven't had lunch yet so I'm doing that first.

0/0
 Reply   Quote More 

 From:  ANT_THOMAS  
 To:  Peter (BOUGHTONP)     
42762.4 In reply to 42762.3 
What was for lunch?
0/0
 Reply   Quote More 

 From:  Peter (BOUGHTONP)  
 To:  ANT_THOMAS     
42762.5 In reply to 42762.4 
A traditional Italian dish.
0/0
 Reply   Quote More 

 From:  koswix  
 To:  Peter (BOUGHTONP)     
42762.6 In reply to 42762.5 
What was on top of it?

 ▪                    
             ┌────┐    ┌────┐                      
          │    │    │    │ ▪                    
          │    └────┘    │                      
          │   ──┐  ┌──   │ ▪                    
   ┌──────┤    ▪    ▪    │                      
  ┌┘      │              │ ▪                    
┌─┤       └──┐  │  │  ┌──┘                      
│ │          │ ││  ││ │   ┌─┐                   
│ │          └─┼┤  └┴─┴───┘ │                   
│ │           ─┘│           │                   
│ │   ┌──────┐  └┬──────────┘                   
  │   │      │   │                              
  │   │      │   │                              
  └───┘      └───┘                              
If Feds call you and say something bad on me, it may prove what I said are truth, they are afraid of it.
0/0
 Reply   Quote More 

 From:  Peter (BOUGHTONP)  
 To:  koswix     
42762.7 In reply to 42762.6 
Mature cheddar cheese - if you believe the packaging - but always an insubstantial amount, and the necessary stirring prevents any noticable taste that those few grams might impart.
0/0
 Reply   Quote More 

 From:  Matt  
 To:  Peter (BOUGHTONP)     
42762.8 In reply to 42762.2 
$db->query (which is a wrapper around mysqli::query) can return false, so adding a call to ->num_rows to it won't always work (as you now already know), but you could check for $result->num_rows afterwards, e.g.:
 
Code: 
if (!($result = $db->query($sql))) return false;

if ($result->num_rows === 0) return false;
0/0
 Reply   Quote More 

 From:  Peter (BOUGHTONP)  
 To:  Matt     
42762.9 In reply to 42762.8 
So I finally got around to checking this properly and confirmed there are just three files where that num_rows check needs to be added - all the others already have suitable code in place.
(Are you ok with me pushing that direct to main GitHub repo, or would you prefer it done as a pull request?)


There's also an issue of get_magic_quotes_gpc being deprecated in PHP 7.x and removed in PHP 8.0

The easy fix for PHP 7.x is to not error on deprecation warnings (except in developer mode), so would you be ok with this change:

diff --git a/forum/boot.php b/forum/boot.php
index e98cf631e..64ecb8a7b 100644
--- a/forum/boot.php
+++ b/forum/boot.php
@@ -47,7 +47,12 @@ require_once BH_INCLUDE_PATH . 'constants.inc.php';
 require_once BH_INCLUDE_PATH . 'errorhandler.inc.php';

 // Set the error reporting level to report all errors
-error_reporting(E_ALL | E_STRICT);
+error_reporting(E_ALL);
+
+// Only show deprecation warnings to developers
+if (!defined("BEEHIVE_DEVELOPER_MODE")){
+    error_reporting(error_reporting() & ~E_DEPRECATED);
+}

 // Enable the error handler
 set_error_handler('bh_error_handler');
?

I think the backwards-compatible fix for PHP 8.x involves switching "get_magic_quotes_gpc()" with "ini_get('magic_quotes_gpc')", but there are also people saying such checks should assume "false" and/or relevant code removed entirely...?

0/0
 Reply   Quote More 

 From:  Matt  
 To:  Peter (BOUGHTONP)     
42762.10 In reply to 42762.9 
This might be a simpler change, without having to sacrifice strict error checking?

Another (arguably better?) way to do it would be to remove the deprecated functionality and bump the minimum supported version to where it is deprecated. That would probably need more testing though.
Code: 
diff --git a/forum/include/server.inc.php b/forum/include/server.inc.php
index a0fe4d715..8a5aac974 100644
--- a/forum/include/server.inc.php
+++ b/forum/include/server.inc.php
@@ -448,8 +448,9 @@ function unregister_globals()

 function disable_magic_quotes()
 {
-    /** @noinspection PhpDeprecationInspection */
-    if (!get_magic_quotes_gpc()) return;
+    if (!function_exists('get_magic_quotes_gpc') || !get_magic_quotes_gpc()) {
+        return;
+    }

     $process = array(
         &$_GET,

 
0/0
 Reply   Quote More 

 From:  Peter (BOUGHTONP)  
 To:  Matt     
42762.11 In reply to 42762.10 
E_STRICT has been included with E_ALL since PHP 5.4 (see PHP docs: Predefined Constants), and I don't think that's an unreasonable minimum version.
(Debian 8 (aka oldoldstable) goes back to PHP 5.6; there are third-party packages for CentOS 7 that provide PHP 5.4 and above, but afaict none for PHP 5.3)

Versions/specifics aside, I don't think E_DEPRECATED should be user-facing - it's a developer warning (so should be on when developing+testing), but is not something an end-user needs to be bothered by.

0/0
 Reply   Quote More 

 From:  Matt  
 To:  Peter (BOUGHTONP)     
42762.12 In reply to 42762.11 
Turning off E_DEPRECATED is fine.

I imagine the next problem we'd run into is users trying to run Beehive on PHP version with the deprecated functionality removed entirely. Beehive doesn't have a strict enough version check, it only does minimum version.

With this specific issue, I would rather remove the functionality and check for magic quotes being turned on and tell the user they need to configure PHP to have it disabled.
0/0
 Reply   Quote More 

Reply to All    
 

1–12

Rate my interest:

Adjust text size : Smaller 10 Larger

Beehive Forum 1.5.2 |  FAQ |  Docs |  Support |  Donate! ©2002 - 2021 Project Beehive Forum

Forum Stats