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

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

Closed
wants to merge 1 commit into from

Conversation

ThauEx
Copy link

@ThauEx ThauEx commented Jul 18, 2021

Details see #294

Copy link
Collaborator

@garak garak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that is supposed to fail with the previous version?

@ThauEx
Copy link
Author

ThauEx commented Jul 19, 2021

Yes, will do

@ThauEx
Copy link
Author

ThauEx commented Jul 20, 2021

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.

@garak
Copy link
Collaborator

garak commented Jul 20, 2021

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.

@ThauEx
Copy link
Author

ThauEx commented Jul 23, 2021

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?

@garak
Copy link
Collaborator

garak commented Jul 23, 2021

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
Copy link
Collaborator

garak commented Dec 4, 2021

Any news on this?

@ThauEx
Copy link
Author

ThauEx commented Dec 5, 2021

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

@ThauEx
Copy link
Author

ThauEx commented Dec 12, 2021

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.

@garak
Copy link
Collaborator

garak commented Dec 12, 2021

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

@garak
Copy link
Collaborator

garak commented Jan 22, 2023

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

@garak garak closed this Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants