knp-components icon indicating copy to clipboard operation
knp-components copied to clipboard

Fix PHP 7.4 deprecation, when using objects with paginate() on Paginator

Open ThauEx opened this issue 2 years ago • 9 comments

Details see #294

ThauEx avatar Jul 18 '21 17:07 ThauEx

Yes, will do

ThauEx avatar Jul 19 '21 12:07 ThauEx

It seems like, the issue only happens with custom EventSubscribers. The existing ones are using slice, which is transforming the object into an array. This means the deprecation message will never show up. Should I create a dummy event for that test case, so the pagination returns an object instead of array? I know, this issue is related to a custom implementation, but it seems like doing it like this is not wrong, so I still thing this is a bug.

ThauEx avatar Jul 20 '21 06:07 ThauEx

I think that we should ~change~ deprecate our current implementation and only accept array or ArrayIterator Any other object does not make sense to be paginated. By the way, isset cannot replace array_key_exists, because it's returning differently when a value is null.

garak avatar Jul 20 '21 06:07 garak

Sorry for my late response. Should I change my code to not use isset or just close it, because it's supposed to work with arrays only anyway?

ThauEx avatar Jul 23 '21 08:07 ThauEx

Sorry for my late response. Should I change my code to not use isset or just close it, because it's supposed to work with arrays only anyway?

Don't worry about the delay, there's no hurry. My suggestion is about restricting possible types for $items to two cases: array and ArrayIterator. Any other type should raise a deprecation for now, and an exception in the next major version. The phpdoc should be changed accordingly.

garak avatar Jul 23 '21 08:07 garak

Any news on this?

garak avatar Dec 04 '21 11:12 garak

Sorry, I was busy... I can have a look next week. I think it could be easy to solve by using typehints.

ThauEx avatar Dec 05 '21 11:12 ThauEx

So before this condition (https://github.com/KnpLabs/knp-components/blob/master/src/Knp/Component/Pager/Paginator.php#L91) there should be a check, whether $itemsEvent->item is an array or ArrayIterator. For now it could be a deprecation message and later a strict check. The best would be to check this inside ItemsEvent (https://github.com/KnpLabs/knp-components/blob/master/src/Knp/Component/Pager/Event/ItemsEvent.php#L29), but since items is a public property, this is not possible.

ThauEx avatar Dec 12 '21 11:12 ThauEx

The latest version already restricts the type of $items to iterable. Again, if you can propose a failing test, it would be better.

garak avatar Dec 12 '21 16:12 garak

I'm closing this since we don't support PHP 7.4 anymore

garak avatar Jan 22 '23 13:01 garak