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

IBX-8470: Upgraded Pagerfanta to v3 for Symfony 6 #462

Merged
merged 27 commits into from
Dec 16, 2024

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Dec 11, 2024

Warning

This is a PR targeting another PR #447

🎫 Issue IBX-8470

Related PRs:

Description:

This PR targets #447 and should be merged into that when/if approved.
We're forced to upgrade Pagerfanta in core to v3 as babdev/pagerfanta-bundle required by AdminUI supports v3+ for Symfony 6+.

There seem to be not a lot of breaking changes, but Pagerfanta v3 adds PHPStan templated type hints and since we extend adapters, create custom ones, and rely inside of those adapters on SearchService value objects, we needed to add/extend/implement those templates in core as well.

I. There are currently at least 2 key design flaws in core:
  1. interface \Ibexa\Core\Pagination\Pagerfanta\SearchResultAdapter extends \Pagerfanta\Adapter\AdapterInterface
    While it was convenient to avoid issues when there was no native support for intersection types, now it created an issue as PF AdapterInterface became a template, therefore Core SearchResultAdapter needed to be one as well, even though none of the methods rely on type T.
  2. ContentSearchAdapter and LocationSearchAdapter extended ContentSearchHitAdapter and LocationSearchHitAdapter respectively.
    Consider, for example, that: ContentSearchAdapter::getSlice() returns
iterable<array-key, \Ibexa\Contracts\Core\Repository\Values\Content\Content>

while ContentSearchHitAdapter::getSlice() returns

iterable<array-key, \Ibexa\Contracts\Core\Repository\Values\Content\Search\SearchHit>

SearchHit and Content are not covariant with respect to each other (their common base is ValueObject) therefore getSlice() returning iterable Content list cannot override the one returning SearchHit list. It worked due to lack of strict types prior Pagerfanta v3.

To resolve that second design flaw I've changed *SearchAdapters to inject relevant *SearchHitAdapters as a composition rather than inherit them. From usage standpoint there's no behavior BC break.

II. SearchService value object templates.

Given our adapters need to extend a templated AdapterInterface, the following SearchService contracts ValueObjects are now also templates:

  • SearchHit due to $valueObject property. This can be ContenInfo, Content, Location, but also their persistence layer counterparts. Therefore declared it as @template-covariant T of \Ibexa\Contracts\Core\Repository\Values\ValueObject. We might want to go into the direction of object there in the future, to extend cearch capabilities.
  • SearchResult as it contains a list of SearchHit instances.
II. Language Filtering types for Search and Filtering.

Not really a must, but since it popped up after the other changes, it might be a good time to introduce PHPStan array shapes: TSearchLanguageFilter and TFilteringLanguageFilter as the variable is often named the same while expecting different inputs.

III. Total count

Pagerfanta v3 defines total count as int<0, max>, so everything relying on that needed to be aligned. Unfortunately FixedSearchResultHitAdapter returns in case of null -1 . That needs to be changed to null value, but it needs to be done carefully and as a follow-up. Therefore added that to the Pagerfanta-dedicated baseline.

IV. Pagerfanta-dedicated baseline

There's still a lot to refactor there and it could probably take a long time. Therefore at some point of endless refactoring I've decided after talking with @Steveb-p to add Pagerfanta-dedicated baseline and resolve as a follow-up later. Some of those issues are probably trivial, so if you have quick ready to apply solution for any of those, feel free to suggest it.

V. Code duplication

No idea how to resolve it ATM. While the code is the same, it operates on different PHPStan templated types, so effectively needs to be duplicated. Unless there's a better way to shape it?

Key changes:

  • [Composer] Bumped pagerfanta/pagerfanta requirement to ^3.6.2
  • [Pagerfanta] Aligned adapters with v3
  • [Pagerfanta] Aligned CopySubtreeCommand with totalCount changes and improved code quality
  • [Pagerfanta][PHPStan] Declared template on Content\Search\SearchResult
  • [Pagerfanta] Aligned totalCount strict type hints (int<0, max>)
  • [Pagerfanta] Defined ContentViewQueryTypeMapper::map strict return type
  • [Pagerfanta][PHPStan] Defined TFilteringLanguageFilter for Filtering API
  • [Pagerfanta][PHPStan] Defined TSearchLanguageFilter for Search API
  • [Pagerfanta] Aligned Core Pagerfanta implementation with Pagerfanta v3
  • [Pagerfanta][PHPStan] Implemented Pagerfanta v3 templates
  • [Pagerfanta] Fixed related classes' code quality and PHPDoc
  • [Pagerfanta] Fixed related implementation code quality
  • [Pagerfanta][Tests] Refactored adapter tests to follow the changes
  • [Pagerfanta][Tests] Improved code quality of related test classes
  • [Pagerfanta][PHPStan] Created Pagerfanta baseline
  • [Pagerfanta][PHPStan] Fixed incorrect persistence findLocations return type
  • [Pagerfanta][PHPStan] Fixed UserService::searchSubGroups return type
  • fixup! [Pagerfanta] Aligned adapters with v3
  • [Pagerfanta]Fixed LSE Handler findContent & findLocation impl. type hints
  • [Pagerfanta] Improved type hints of QueryRenderController
  • [Pagerfanta][Tests] Improved QueryRenderControllerTest
  • [Pagerfanta] Fixed incorrect return types of SearchHitAdapterFactory methods
  • [Pagerfanta][PHPStan] Removed resolved issues from the main baseline
  • [Pagerfanta][PHPStan] Aligned Pagerfanta baseline with resolved issues

@alongosz alongosz force-pushed the ibx-8470-pagerfanta branch from c59072b to 7d97e72 Compare December 11, 2024 22:11
@alongosz alongosz marked this pull request as ready for review December 12, 2024 12:31
@alongosz alongosz requested review from Steveb-p and a team December 12, 2024 12:32
src/bundle/Core/Command/CopySubtreeCommand.php Outdated Show resolved Hide resolved
src/bundle/Core/Command/CopySubtreeCommand.php Outdated Show resolved Hide resolved
@konradoboza konradoboza requested a review from a team December 13, 2024 07:16
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
4.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@alongosz alongosz merged commit d3cb362 into ibx-8470-symfony-6 Dec 16, 2024
12 of 14 checks passed
@alongosz alongosz deleted the ibx-8470-pagerfanta branch December 16, 2024 09:09
alongosz added a commit that referenced this pull request Dec 27, 2024
For the list of Pagerfanta changes see #462


---------

Co-Authored-By: Dawid Parafiński <ViniTou@users.noreply.github.com>
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.

5 participants