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 interface for ContentRichEntityRepository #268

Draft
wants to merge 4 commits into
base: 0.9
Choose a base branch
from

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Aug 9, 2024

We require a minimal interface which all content rich entity need to implemented. This interface should then be used to provide basic implementations of:

  • ContentRouteDefaultsProvider
  • TeaserProvider
  • LinkProvider
  • SmartContentProvider
  • Selection
  • Single Selection

It should also replace the whole DimensionContentRepository which we sadly use at a lot of cases.

@alexander-schranz alexander-schranz force-pushed the feature/content-rich-entity-repository branch from e1e76e1 to b112b18 Compare August 9, 2024 10:58
@coveralls
Copy link

coveralls commented Aug 9, 2024

Pull Request Test Coverage Report for Build 10318437173

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 10299929317: 0.0%
Covered Lines: 2532
Relevant Lines: 2532

💛 - Coveralls

Copy link
Member Author

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

@Prokyonn what do you think about the interface?

* @template T of ContentRichEntityInterface
*
* @phpstan-type ContentRichEntityRepositoryFilters array{
* ids: array<string>,
Copy link
Member Author

Choose a reason for hiding this comment

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

this may is a ids or uuids so not sure about how we handle this. Should we go with identifiers @Prokyonn

Copy link
Member Author

Choose a reason for hiding this comment

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

The ContentRichEntityInterface has a getId():

but now that we said may Identifier make more sense in case of id vs uuid. But another option is as we already force getId in the interface keep ids and may also just rename Artice:uuid to Article::id even if its a uuid. /cc @chirimoya

Copy link
Member

Choose a reason for hiding this comment

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

Technically speaking, id is just the abbreviation of identifier 😅
also writing ['identifier' => xx] feels more cumbersome than 'id' => xx 🤔

Although we may use uuid, I think I would still go with id as a string.

Comment on lines 23 to 32
* locale?: string|null,
* stage?: string|null,
* categoryIds?: int[],
* categoryKeys?: string[],
* categoryOperator?: 'AND'|'OR',
* tagIds?: int[],
* tagNames?: string[],
* tagOperator?: 'AND'|'OR',
* templateKeys?: string[],
* loadGhost?: bool,
Copy link
Member Author

Choose a reason for hiding this comment

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

ids / identifiers is the only when which need be implemented the other filters are provided via the QueryEnhancer:

            $this->dimensionContentQueryEnhancer->addFilters(
                $queryBuilder,
                'example',
                ExampleDimensionContent::class,
                $filters,
                $sortBys
            );

@alexander-schranz alexander-schranz force-pushed the feature/content-rich-entity-repository branch from d22638b to b4cc47b Compare August 9, 2024 11:14
@alexander-schranz alexander-schranz added the Feature New functionality not yet included in Bundle label Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality not yet included in Bundle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants