3

I updated to PHP 7.2 and it created an array (no pun intended) of issues. I've been knocking them out (mostly these sizeof and count() warnings. The one error we have :

Warning: sizeof(): Parameter must be an array or an object that implements Countable in /usr/www/domain/phpmyd/includes/class_registry.php on line 236

I tried fixing it like this :
if (sizeof($this->config) < 1) {
To this:
if (!empty($this->config) &&(sizeof($this->config) < 1)) {

But it creates lots more errors shown below, However, we fixed this one in the same way and it works perfectly. Changing this:
if (0 < sizeof($this->language)) {
To this:
if (!empty($this->language) && (0 < sizeof($this->language))) {

Took away basically the same error. Now, keep in mind, the above warning is the ONLY error left. Everything else works perfectly, however, if I do "fix" the warning, I get a bunch of errors that break the site and seem irrelevant. So, if I replace that first string all these errors appear:

  • Warning: Use of undefined constant ADDON_DISCOUNT_CODES - assumed 'ADDON_DISCOUNT_CODES' (this will throw an Error in a future version of PHP) in /usr/www/domainlistings/phpmyd/index.php on line 6
  • Warning: Use of undefined constant ADDON_BLOG - assumed 'ADDON_BLOG' (this will throw an Error in a future version of PHP) in /usr/www/domainlistings/phpmyd/cp/template/default/admin_header.tpl on line 134
  • Warning: Use of undefined constant ADDON_LINK_CHECKER - assumed 'ADDON_LINK_CHECKER' (this will throw an Error in a future version of PHP) in /usr/www/domainlistings/phpmyd/cp/template/default/admin_header.tpl on line 179

Those errors did NOT appear, and those things worked perfectly well until I changed if (sizeof($this->config) < 1) {

How is this linked? I'm not sure what is happening here, how that one line can make or break these other (seemingly irrelevant) things. Full code of inital problem (line 236):

/**
     * Get a configuration value
     * @param string $key
     * @return mixed
     */
    public function getConfig($key) {
        if (sizeof($this->config) < 1) {
            $this->loadConfig();
        }
        return isset($this->config[$key]) ? $this->config[$key] : false;
    }

Any ideas?

Sachith Muhandiram
  • 2,819
  • 10
  • 45
  • 94
Natsu
  • 111
  • 1
  • 2
  • 11
  • 1
    Just curious: why do you need sizeof/count here? What's wrong with a simple `if (!$this->config)`? – user1597430 Nov 19 '19 at 23:15
  • OP was asking why's this not working. – Mike Doe Nov 19 '19 at 23:15
  • 1
    This is a common issue with the update to 7.2, especially with contrib modules / packages. As of 7.2.0, count throws a warning if passed a null value, it used to fail silently. e.g. https://www.php.net/manual/en/migration72.incompatible.php#migration72.incompatible.warn-on-non-countable-types – JDev518 Nov 19 '19 at 23:28
  • @JDev518 That is correct, and I've fixed a dozen of these already, but this one seemed to have consequences that were mostly unlinked, and I didn't know the cause (or the link). Emix gave me some things to think about! – Natsu Nov 19 '19 at 23:31

4 Answers4

4

For starters don't use sizeof, but count ;) cheap improvement - always one opcode less.

Secondly, make sure you pass an array, not null or whatever you have there, eg.:

// dirty fix
if (count((array) $this->config) > 0) {
    …
}

To fix this properly you should never allow this property to be null anyway. If you're after some lazy loading check if the variable is null or is not an array, eg:

public function getConfig($key, $default = false)
{
    if (!is_array($this->config)) {
        // make sure it becomes one after the load
        $this->loadConfig();
    }

    return $this->config[$key]) ?? $default;
}
Mike Doe
  • 16,349
  • 11
  • 65
  • 88
  • So, you're not wrong with the second part. That does seem a more valid approach, though if I implement a solution like that it throws the same config errors and we unfortunately are blanking on why, Ill have to really think about this. The first fix DOES stop the warning and doesn't break the config though. I am honestly shocked that works like it does, and we didn't think about it. Both your answers are valid, I appreciate it! I – Natsu Nov 19 '19 at 23:29
2

I think what's happening here is that the change you originally made ensures that loadConfig() never happens, and something in loadConfig() is responsible for defining all those constants you're getting warned about after changing that.

If you changed

if (sizeof($this->config) < 1) {

to

if (!empty($this->config) &&(sizeof($this->config) < 1)) {

then if $this->config was null (the default value of an object property that has not yet been given a value), then the second one would mean the if condition wouldn't be satisfied because null is empty, where in the first one, it would be satisfied because sizeof(null) still returns 0 even though it gives you the uncountable warning.

sizeof was never really necessary there. You don't have to count something just to see if it exists.

I think this should work just fine, and make more sense for what it's actually meant to do.

if (empty($this->config)) {
    $this->loadConfig();
}
Don't Panic
  • 41,125
  • 10
  • 61
  • 80
  • Thank you! That makes a lot of sense, I really appreciate you breaking it down like that. There's definitely other places for imporvement too, thank you! – Natsu Nov 20 '19 at 00:10
2

You can just check, before passing the variable, if it's an array or not:

if(is_array($this->config)){
   if(sizeof($this.config) < 1) {
       // code here
   }
}
Zain
  • 21
  • 3
2

These days you can use the null coalescing operator (??) to help in this scenario; i.e. by using it to take the given value if it's not null, or taken an empty array when the given value is null.

So your code: if (sizeof($this->config) < 1) { ...would become: if (sizeof($this->config ?? []) < 1) {.

JohnLBevan
  • 22,735
  • 13
  • 96
  • 178