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

Add final for every method of abstract classes #7108

Merged
merged 2 commits into from
May 2, 2021

Conversation

VincentLanglet
Copy link
Member

Subject

I am targeting this branch, because BC.

Closes #5791. (I think)

Changelog

### Added
- `final` for method of every abstract classes.

@VincentLanglet VincentLanglet added this to the 4.0 milestone Apr 23, 2021
@VincentLanglet
Copy link
Member Author

I need to check if the haveToPaginate override in the Simple Pager could be removed.

@VincentLanglet
Copy link
Member Author

I need to check if the haveToPaginate override in the Simple Pager could be removed.

Maybe you can help me @franmomu since you're using the Simple Pager ; do you see any differences if you remove the override of the method ?

@franmomu
Copy link
Member

I need to check if the haveToPaginate override in the Simple Pager could be removed.

Maybe you can help me @franmomu since you're using the Simple Pager ; do you see any differences if you remove the override of the method ?

I haven't checked in a project and I guess it would be the same, but in SimplePager I guess it's implemented that way for performance reasons.

@VincentLanglet
Copy link
Member Author

I haven't checked in a project and I guess it would be the same, but in SimplePager I guess it's implemented that way for performance reasons.

Since the countResults method is not doing any queries:

    public function countResults(): int
    {
        $n = ($this->getLastPage() - 1) * $this->getMaxPerPage();
        if ($this->getLastPage() === $this->getPage()) {
            return $n + $this->thresholdCount;
        }

        return $n;
    }

I don't see real perfomance improvements.

@VincentLanglet VincentLanglet requested a review from a team April 24, 2021 20:51
@franmomu
Copy link
Member

When iterating results:

public function getCurrentPageResults(): iterable
{
if (null !== $this->results) {
return $this->results;
}
$this->results = $this->getQuery()->execute();
$this->thresholdCount = \count($this->results);
if (\count($this->results) > $this->getMaxPerPage()) {
$this->haveToPaginate = true;
if ($this->results instanceof ArrayCollection) {
$this->results = new ArrayCollection($this->results->slice(0, $this->getMaxPerPage()));
} else {
$this->results = new ArrayCollection(\array_slice($this->results, 0, $this->getMaxPerPage()));
}
} else {
$this->haveToPaginate = false;
}
return $this->results;
}

this->haveToPaginate is set, so there is no need to even make any call, I haven't check the real performance though.

@VincentLanglet
Copy link
Member Author

this->haveToPaginate is set, so there is no need to even make any call, I haven't check the real performance though.

I agree, but only a multiplication and a sum is done, so it's not significant.

@jordisala1991
Copy link
Member

I need to check if the haveToPaginate override in the Simple Pager could be removed.

IMO we should not mark it final if the simple pager overrides it, have you tried performance implications of it?

@VincentLanglet
Copy link
Member Author

VincentLanglet commented Apr 26, 2021

IMO we should not mark it final if the simple pager overrides it, have you tried performance implications of it?

Previously it was

    public function haveToPaginate()
    {
        return $this->haveToPaginate || $this->getPage() > 1;
    }

Now it's

        $n = ($this->getLastPage() - 1) * $this->getMaxPerPage();
        if ($this->getLastPage() === $this->getPage()) {
            return $n + $this->thresholdCount;
        }

        return $n > $this->getMaxPerPage();

It only cost a multiplication ; there is no impact.

@VincentLanglet
Copy link
Member Author

Please review @sonata-project/contributors

@VincentLanglet VincentLanglet requested a review from a team May 1, 2021 19:28
@jordisala1991 jordisala1991 merged commit 495bcb8 into sonata-project:3.x May 2, 2021
@jordisala1991
Copy link
Member

Thank you @VincentLanglet

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

Successfully merging this pull request may close these issues.

[Next Major] Close API
4 participants