php8.1 changes - don't call unserialize with null, also add ReturnTypeWillChange attribute to CActiveRecord's count method

[kenguest]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Tests pass?	✔️
Fixed issues	comma-separated list of tickets # fixed by the PR, if any

[marcovtwout]

@kenguest Which interface requires the ReturnTypeWillChange additions? Also please check formatting (tabs instead of spaces)

[kenguest]

@kenguest Which interface requires the ReturnTypeWillChange additions? Also please check formatting (tabs instead of spaces)

CActiveRecord's count, countByAttributes, and countBySql methods required ReturnTypeWillChange annotations before I could run my app against PHP 8.1

[rob006]

CActiveRecord's count, countByAttributes, and countBySql methods required ReturnTypeWillChange annotations before I could run my app against PHP 8.1

Why? I don't see why it would be needed.

Also, did you add all these guards because of actual errors? I checked few cases and I don't see how you could get null there. For example CSecurityManager::validateData() should never return null, so why would you add guard for such case? Many cache or storage related cases always call serialize() on set, so you should never get null when you want to get data from storage. And even if you get null, it is probably a configuration error and ignoring these may hide some errors and make debugging harder.

[kenguest]

@rob006 I've reverted the guard-checks re calling unserialize with null args for e.g. CSecurityManager::validateData. I was perhaps too free with adding them. Checks in CCache::get() are still required however.

Going back to my adding ReturnTypeWillChange attributes - this is to fix issues such as what you can see here, without changing the signatures of those methods: Return type of CActiveRecord::count($condition = '', $params = []) should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice (/var/www/project1/vendor/yiisoft/yii/framework/yiilite.php:8684

[rob006]

CActiveRecord does not implement Countable, so I'm not sure why you get such errors.

Checks in CCache::get() are still required however.

Why? In what scenario you get such errors? The same question applies to state persisters and message sources.

[kenguest]

In the case of CCache it's due to its implementing the ArrayAccess interface - used in the prime example in the documentation at https://php.watch/versions/8.1/ReturnTypeWillChange

Do you need me to find you a demo project for you to encounter these issues yourself? :-)

[rob006]

In the case of CCache it's due to its implementing the ArrayAccess interface - used in the prime example in the documentation at https://php.watch/versions/8.1/ReturnTypeWillChange

Your PR does not add ReturnTypeWillChange to CCache.

Do you need me to find you a demo project for you to encounter these issues yourself? :-)

We have tests for that - unit test will be enough.

[kenguest]

In the case of CCache it's due to its implementing the ArrayAccess interface - used in the prime example in the documentation at https://php.watch/versions/8.1/ReturnTypeWillChange

Your PR does not add ReturnTypeWillChange to CCache.

True. That's because @marcovtwout added ReturnTypeWillChange to CCache. :-) My change was to add a guard test to ensure unserialize isn't called with a null argument as that was occurring in the get method with one of the legacy projects that I maintain for my employers.

[marcovtwout]

@kenguest Thank you for your contribution, but after reading above review it looks like this mostly fixes mis-usage of framework code in a specific project that perhaps was unnoticed before PHP 8.1.

@rob006 am I summarizing correctly or do you see parts that are still valid for 8.1 compatibility?

[kenguest]

@marcovtwout the fixes that addressed misusage of the framework have been reverted since my initial commit - what remains are still required for PHP 8.1 compatibility, especially the annotations I added for framework/db/ar/CActiveRecord.php

[marcovtwout]

@kenguest but #[\ReturntypeWillChange] is only needed when implementing an interface and not adhering to the parent spec, what interface is CActiveRecord implementing that requires this annotation?

[rob006]

@kenguest Can you show a scenario where you get such errors and these guards will help? It looks more like a blind guards protecting against situations that should never happen.

[kenguest]

If I remove the ReturnTypeWillChange annotation which I added for the CActiveRecord::count method, the following error is displayed and logged to the application.log file: 2022/09/28 14:32:13 [error] [php] Return type of CActiveRecord::count($condition = '', $params = []) should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice (/var/www/project/vendor/yiisoft/yii/framework/yiilite.php:8684)

I've removed the ReturnTypeWillChange annotation from the countByAttributes and countBySql methods, as it is only needed for the count method

[rob006]

@kenguest Do you implement Countable in one of your models?

[kenguest]

@rob006 - thanks for the gentle nudge in the right direction.

One of the models had indeed been declared with "implements Countable", but did not have its own count method, which gave rise to the error I included in my earlier comment.

Once I removed "implements Countable", without the annotation in place on CActiveRecord's count method, the error didn't occur.

Sorry for taking up your time on this. :/

[marcovtwout]

Glad all is figured out :) Closing.