Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seeing PHP errors with PHP 7.2 #408

Closed
gmclelland opened this issue Oct 17, 2017 · 55 comments
Closed

Seeing PHP errors with PHP 7.2 #408

gmclelland opened this issue Oct 17, 2017 · 55 comments

Comments

@gmclelland
Copy link

gmclelland commented Oct 17, 2017

Short description of the issue

If I navigate to the modules page, I see the following errors:

1x | PHP Warning: count(): Parameter must be an array or an object that implements Countable in .../Inputfield/InputfieldFile/InputfieldFile.module:637
--
1× | PHP Warning: count(): Parameter must be an array or an object that implements Countable in .../Inputfield/InputfieldFile/InputfieldFile.module:229

Expected behavior

No errors will be shown.

Actual behavior

Errors are shown.

Optional: Suggestion for a possible fix

@adrianbj suggested "Looks like we need to explicitly do an is_array() check before count() now."

Steps to reproduce the issue

  1. Go to the modules page
  2. View error with TracyDebugger shown in the backend

Setup/Environment

Apache/2.4.28 (Unix) PHP/7.2.0beta3

  • ProcessWire version:
    3.0.79
@adrianbj
Copy link

@adrianbj suggested "Looks like we need to explicitly do an is_array() check before count() now."

Just to clarify, is_array() if an array is expected, or if it's an object, then you need to check if it implements countable

@szabeszg
Copy link

more on this: https://wiki.php.net/rfc/counting_non_countables

@adrianbj
Copy link

Maybe also worth a read: https://blog.martinhujer.cz/php-7-2-is-due-in-november-whats-new/ in case there might be other things that need attention.

@ryancramerdesign
Copy link
Member

This isn't consistent with the PHP count() documentation. It states that the argument can be mixed, so you can pass anything into it. It also outlines what the return value behavior is:

When the parameter is neither an array nor an object with implemented Countable interface, 1 will be returned. There is one exception, if array_or_countable is NULL, 0 will be returned.

The page even includes examples of using it with null and false, and it's the null version that I use quite often since PW often uses null for values known not yet populated, and blank array for populated but empty. We expect count() to return 0 for either, as it states. We also rely on count() to return 1 for Wire derived objects that aren't WireArray, since they represent a single item that can be stored in a WireArray... because it's "1" item. Short term will stick to the documentation until they change it. But it sounds like this less useful version of count() may end up in the final PHP 7.2, so we'll have to add a wireCount() function that duplicates the currently documented behavior of count(). Either that or wrap every count() call with multiple if() and else statements... but I think wireCount sounds better to me.

@adrianbj
Copy link

Did you also see this: http://php.net/manual/en/migration72.incompatible.php

I am dealing with this at the moment and it's not always a simple solution - I have relied on count() in different ways and there isn't a stock approach that can be applied, at least in my experience so far.

The thing I was almost tempted to do was to cast the variable inside the count() as an array, eg: count((array)$var). I feel like that would probably always work, but it seems wrong/dirty and actually avoids the reason they have made this change in the first place - detection of hidden bugs by relying on count to do something that semantically it shouldn't be able to do.

I am curious how see wireCount() handling wireCount('string'); - will it return 1 as per the current behavior, or false?

@ryancramerdesign
Copy link
Member

ryancramerdesign commented Oct 19, 2017

I can't say I've ever had an issue with bugs and using count(), and actually find the behavior handy. It's like they've changed the purpose of the function. I don't agree that semantically count() should only be able to count certain types. I mean, if you give me 1 apple in the real world, then I've got 1 apple, whether you gave it to me in a bag that can hold a dozen apples, or you just gave me an apple. If you give me nothing (null) then I've got nothing (0).

I am curious how see wireCount() handling wireCount('string'); - will it return 1 as per the current behavior, or false?

I'm not sure I understand why it would ever return false? :) If we're asking for a count() we're asking for an integer. So if you give me one "string", then I've got 1 string (return value 1). I've never perceived PHP's count() to be something like Javascript.length property, where it can return the length of a string or the count of an array/object. That would be a whole different function I'd think, like a mashup of strlen() and count().

Here's a potential implementation of wireCount():

/**
 * Return the count of item(s) present in the given value
 * 
 * Duplicates behavior of PHP count() function prior to PHP 7.2, which states:
 * Returns the number of elements in $value. When the parameter is neither an array nor an 
 * object with implemented Countable interface, 1 will be returned. There is one exception, 
 * if $value is NULL, 0 will be returned.
 * 
 * @param mixed $value
 * @return int
 * 
 */
function wireCount($value) {
	if($value === null) return 0; 
	if(is_array($value)) return count($value); 
	if(is_object($value) && $value instanceof \Countable) return count($value);
	return 1;
}

@adrianbj
Copy link

adrianbj commented Oct 19, 2017

actually find the behavior handy

I agree it's handy and I also rely on it quite often. Whether it is semantic or not I guess is up to interpretation - I agree that with a real world analogy it sounds fine, but not sure if that translates to code or not.

Interesting quote in one of those links:

I really like this change, because no one writes such code intentionally, but rather as a bug that needs fixing.

While I don't agree that no ones writes such code intentionally, I do agree that the change can help to identify bugs if you pass in a $str mistakenly thinking it is an array. Before it would have returned 1 and you wouldn't know things were not as expected. Yes the new behavior requires things to be more explicit and it's a bit of a pain, but it might actually be better in the end and also create more understandable code. Maybe? - I don't know :)

@teppokoivula
Copy link

teppokoivula commented Oct 19, 2017

... but it might actually be better if the end and also create more understandable code. Maybe? - I don't know :)

At least it makes one thing more obvious: without reading the docs I actually would've expected count() to behave like .length when used on strings.

In my humble opinion returning 1 for a string value of any length (or any given number, including 0, for that matter) may be handy once you know it, but still feels a bit weird :)

@ryancramerdesign
Copy link
Member

At least it makes one thing more obvious: without reading the docs I actually would've expected count() to behave like .length when used on strings.

It's the opposite for me, but I was also using PHP for years before I knew any Javascript. I think it just depends what you get used to. For me count() is a handy function that behaves exactly how I'd like it to (prior to PHP 7.2 anyway). It just simply returns a count of how many items you give it, whether it's 1 string (which is 1 item), 1 boolean (1 item), 1 non-Countable object (1 item), an array of 5 integers (5 items), a Countable with 10 pages (10 items), or the one handy exception: null (no items).

@gmclelland
Copy link
Author

FYI... Just reporting that I noticed the new commits lately, so I tried out ProcessWire 3.0.80, but I still see the same errors when I visit the modules page.

@adrianbj
Copy link

FYI... Just reporting that I noticed the new commits lately, so I tried out ProcessWire 3.0.80, but I still see the same errors when I visit the modules page.

Ryan added the new wireCount() function, but hasn't yet applied it to any of the core code - assuming that will come with time - probably as he edits various scripts containing regular count()

@adrianbj
Copy link

adrianbj commented Dec 6, 2017

Just a reminder that php 7.2 was made the current stable a week ago :)

ryancramerdesign added a commit to processwire/processwire that referenced this issue Dec 8, 2017
@ryancramerdesign
Copy link
Member

Okay sounds like a good time to start using wireCount(). I've updated the two calls referenced in this issue report. Please let me know if you come across any others. I need to update to 7.2 here as well.

@adrianbj
Copy link

adrianbj commented Dec 8, 2017

Thanks Ryan - I'll update to the dev version today and then see what else shows up and list here as I find the. FYI there are a few cases of this within FormBuilder as well.

@adrianbj
Copy link

adrianbj commented Dec 8, 2017

Here's that FB one:

.../site/modules/FormBuilder/ProcessFormBuilder.module:2103

@adrianbj
Copy link

.../wire/modules/Inputfield/InputfieldMarkup.module:64

@adrianbj
Copy link

.../wire/modules/Inputfield/InputfieldForm.module:407

@mustafa-online
Copy link

Warning: count(): Parameter must be an array or an object that implements Countable in .../../ProcessPageListerPro\ListerProConfig.php on line 119

@lmd-code
Copy link

lmd-code commented Feb 5, 2018

I found another case:
Warning: count(): Parameter must be an array or an object that implements Countable in ...\wire\modules\Fieldtype\FieldtypeOptions\SelectableOptionConfig.php on line 114

NB: PW version: 3.0.90 (DEV)

ryancramerdesign added a commit to processwire/processwire that referenced this issue Feb 5, 2018
@ryancramerdesign
Copy link
Member

ryancramerdesign commented Feb 5, 2018 via email

@adrianbj
Copy link

Another one here:
Warning: count(): Parameter must be an array or an object that implements Countable in ../wire/modules/Fieldtype/FieldtypeURL.module on line 73

@adrianbj
Copy link

And another:
Warning: count(): Parameter must be an array or an object that implements Countable in .../Fieldtype/FieldtypeOptions/FieldtypeOptions.module:109

ryancramerdesign added a commit to processwire/processwire that referenced this issue Feb 26, 2018
@ryancramerdesign
Copy link
Member

Thanks I've pushed updates for the ones mentioned above. I think it's fine to keep this report going for PHP 7.3 as well.

@Lazerproof
Copy link

@ryancramerdesign I got:

: count(): Parameter must be an array or an object that implements Countable in on line .

when trying to access Parent properties in Custom find (Selector field) white configure page reference field.

2018-12-20_17 00 11_s5vuu

PW 3.0.122 and PHP 7.2.4

@ryancramerdesign
Copy link
Member

@Lazerproof I'm not seeing that one in PHP 7.2, so wondering if it might be coming from a specific Fieldtype (3rd party or core) that I don't have installed. Does the error message indicate a file or line number?

@Lazerproof
Copy link

@ryancramerdesign I'm on almost clean installation: I have installed only RepeaterMatrix v4.

PHP Warning: count(): Parameter must be an array or an object that implements Countable in ...\InputfieldSelector\InputfieldSelector.module:782

ryancramerdesign added a commit to processwire/processwire that referenced this issue Dec 20, 2018
@ryancramerdesign
Copy link
Member

@Lazerproof Not being able to duplicate it here, I'm not sure what the conditions are that are leading to it, but having the file and line number makes it an easy fix. I have changed that count() to a wireCount() which should fix it. Thanks.

@netcarver
Copy link
Collaborator

@Lazerproof Could you confirm if the last code pushed by Ryan fixes this for you?

@netcarver
Copy link
Collaborator

netcarver commented Feb 16, 2019

@ryancramerdesign Maybe grep'ing the entire codebase for count()/strpos() could flag all candidates for fixes in one shot. Might lead to a long list to work through.

@ryancramerdesign
Copy link
Member

@netcarver I'd rather have folks report these kinds of issues when they appear because the vast majority of of these function calls should remain as-is. There's only been a few specific ones that needed adjustment, and I think there's a decent chance we won't see many (if any) more of these.

@reminders
Copy link

reminders bot commented Feb 21, 2019

@netcarver set a reminder for Mar 21st 2019

@netcarver
Copy link
Collaborator

Closing this issue now. Feel free to post more if you come across this again.

@reminders
Copy link

reminders bot commented Mar 21, 2019

👋 @netcarver, review this issue

jdiderik added a commit to jdiderik/processwire that referenced this issue Jul 10, 2019
* 'master' of https://github.com/processwire/processwire: (273 commits)
  Bump version to 3.0.123
  Update for processwire/processwire-issues#408 InputfieldSelector count() to wireCount()
  Add $options argument support to WireMailTools::new(), i.e. $mail->new(array $options)
  Add additional mbstring check to Sanitizer and add two new whitespace reduction methods (for internal use)
  Fix issue processwire/processwire-issues#724 UTF-8 pagenames in non-default language
  Update for processwire/processwire-issues#767
  Bump version to 3.0.122 plus some other minor adjustments
  Update WireArray::import() to support duplicate items when duplicate checking is turned off per processwire/processwire-issues#767
  Fix issue processwire/processwire-issues#766 auto-remove UTF-8 value of `&#8232` line-seperator entity in $sanitizer->text() function
  Add @horst-n fix for processwire/processwire-issues#715
  Fix issue processwire/processwire-issues#763 for ProcessRole, plus this commit also contains some minor tweaks to ProcessPageLister
  Fix issue with Safari column widths in AdminThemeUikit that use showIf conditions per processwire/processwire-issues#480
  Fix issue processwire/processwire-issues#756 make link modal files selection not require being open before it can be populated with files from page selection
  Fix issue processwire/processwire-issues#723
  Fix issue processwire/processwire-issues#749
  A couple of updates to account for new PHP 7.2/7.3 notices per processwire/processwire-issues#408
  Various minor tweaks/updates and bump version to 3.0.121
  Update locale warning message in ProcessLogin per processwire/processwire-issues#732
  Update phpdoc @return statement for Page::index()
  Additional updates for processwire/processwire-issues#751 plus some enhancements to PageFinder
  ...
@ghost
Copy link

ghost commented Aug 30, 2019

Okay sounds like a good time to start using wireCount(). I've updated the two calls referenced in this issue report. Please let me know if you come across any others. I need to update to 7.2 here as well.

The wireCount fixed the error message for me.
thankx

@eladnova
Copy link

eladnova commented Feb 4, 2020

I'm getting a count() error on an old site. Client just informed me and it must have been there for a while.

Warning: count(): Parameter must be an array or an object that implements Countable in /var/www/vhosts/domain.com/httpdocs/site/assets/cache/FileCompiler/site/modules/MarkupSEO/MarkupSEO.module on line 245

I've upgraded to 3.0.148 and PHP 7.3.13 and the error persists.

@teppokoivula
Copy link

@eladnova, it seems to me that this is not a core issue, but rather a problem with the MarkupSEO module. Additionally it seems that there's a fork of the module where this issue is fixed — check this out for details: nicoknoll/MarkupSEO#38.

@adrianbj
Copy link

@ryancramerdesign - just noticed another one of these in ListerPro
image

@Toutouwai
Copy link

@adrianbj, I believe that is fixed in the current ListerPro - maybe you have an older version?

@adrianbj
Copy link

Thanks @Toutouwai - you're right as usual :) I thought I was up to date on all site, but apparently not this one. We really need Pro modules to be serviced by the PW Upgrades module so we can see when they are out of date.

@Toutouwai
Copy link

We really need Pro modules to be serviced by the PW Upgrades module so we can see when they are out of date.

Absolutely. Some Pro modules such as FormBuilder and ProCache are already set up in the Modules directory - we just need the others like ListerPro and ProFields set up there as well (which I have requested in the past).

@szabeszg
Copy link

We really need Pro modules to be serviced by the PW Upgrades module so we can see when they are out of date.

Ryan used to mention support for it, as far as I can remember. I hope it is not at the bottom of his todo list :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests