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

Compatibility up to PHP8.4 #572

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kohlerdominik
Copy link

This PR resolves deprecation warnings in PHP8.4 and adds tests for versions up to PHP8.4.

It includes changes from #568 for simplicity.

@@ -73,7 +73,7 @@ public function getTotal(): int
*/
public function getCount(): int
{
return $this->paginator->getIterator()->count();
return $this->paginator->count();
Copy link
Contributor

Choose a reason for hiding this comment

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

->getTotal() returns the total number of results while ->getCount() should return the number of results in the current page.

Suggested change
return $this->paginator->count();
return $this->paginator->getIterator()->count();

Copy link
Author

Choose a reason for hiding this comment

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

This is a fix from @benepfist, which I wrongfully assumed to be correct.

I reverted the change. Because the documented return type of the base method in Doctrine being Traversable, phpstan will complain here. As we know, that this is an ArrayIterator, we can skip the warning from phpstan imo. If this changes at some point, it SHOULD throw an error and a new solution has to be found for the to-be implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

A better solution would be something like:

$iterator = $this->paginator->getIterator();
if (is_countable($iterator)) {
    return count($iterator);
}

return $this->paginator->count();

Copy link
Author

@kohlerdominik kohlerdominik Dec 12, 2024

Choose a reason for hiding this comment

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

today I learned of the iterator_count() function. Added a new approach using this.

Your's would work aswell; if you prefer this after considering my suggestion, I will update to your approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, though I wonder about the implication of seeking the iterator: https://3v4l.org/gQEB5

Copy link
Author

Choose a reason for hiding this comment

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

It's a valid point and I thought about always cloning the instance for this reason:

return iterator_count(clone $this->paginator->getIterator());

but I feel like this could be way to expensive for to little benefit.

So I went back to your suggestion checking for is_countable() and using count(), which with the current Doctrine version uses the ->count() method on the ArrayIterator:

  • If implementation would change to either another \Countable iterator or even a plain array, this would still be fully functional and performant.
  • In case the iterator be replaced with another \Traversable that is not implementing \Countable this would fallback to use iterator_count() with a cloned instance of the iterator - a little more expensive, but the benefits of having the correct result with unaffected iterator (thinking about NoRewindIterator) are worth the cost IMO. This path is to handle a rather unlikely fallback case.

I hope you can agree on this suggestion. If not - please provide the implementation you'd like and I will gladly do it that way. None of my projects uses Doctrine anyway, so I'm rather uninformed about the internals.

@kohlerdominik kohlerdominik force-pushed the compatibility/php84 branch 2 times, most recently from 424a99a to 2240019 Compare December 13, 2024 12:47
@calebdw
Copy link

calebdw commented Jan 2, 2025

Hello!

Can this be released? The deprecations are filling up the log files...

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants