Skip to content

Add content resolver for the new ContentBundle architecture #7611

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

Merged
merged 21 commits into from
Nov 25, 2024

Conversation

Prokyonn
Copy link
Member

@Prokyonn Prokyonn commented Oct 14, 2024

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #
Related issues/PRs #
License MIT
Documentation PR sulu/sulu-docs#

What's in this PR?

Adds new PropertyResolver and ResourceLoader to use for the ContentBundle

Why?

ContentBundle will introduce a new architecture to more efficiently resolve content.

To Do

  • Create a documentation PR
  • Add breaking changes to UPGRADE.md

@alexander-schranz
Copy link
Member

alexander-schranz commented Oct 15, 2024

As discussed with @Prokyonn teaser and link content type are a bit differently. But we said that we will go with a LinkLoader and TeaserLoader and so a special "link" and "teaser" resource, that way we don't require a lot of changes for teaser provider and link providers and can keep them currently as they are. But all teasers and links should be loaded via one query per type that way.

@alexander-schranz
Copy link
Member

@Prokyonn for the PropertyResolvers inside bundle I would remove the Resolver namespace think PropertyResolver is enough. For ContentBundle it make sense to have that namespace for implementation I would skip it. What do you think?

-Sulu\Bundle\ContactBundle\Infrastructure\Sulu\Content\PropertyResolver\Resolver\AccountSelectionPropertyResolver
+Sulu\Bundle\ContactBundle\Infrastructure\Sulu\Content\PropertyResolver\AccountSelectionPropertyResolver

@Prokyonn
Copy link
Member Author

@Prokyonn for the PropertyResolvers inside bundle I would remove the Resolver namespace think PropertyResolver is enough. For ContentBundle it make sense to have that namespace for implementation I would skip it. What do you think?

-Sulu\Bundle\ContactBundle\Infrastructure\Sulu\Content\PropertyResolver\Resolver\AccountSelectionPropertyResolver
+Sulu\Bundle\ContactBundle\Infrastructure\Sulu\Content\PropertyResolver\AccountSelectionPropertyResolver

Yes that totally makes sense 👍

@mamazu
Copy link
Contributor

mamazu commented Oct 22, 2024

Just as a random thought. Could you also design the system so that it is easy to profile and debug these functions simmilar to the DocumentManager/DebugEventDispatcher?

@alexander-schranz
Copy link
Member

@mamazu as now all are service tags it is straight forward over the debug:container now:

  • Get All property resolvers: bin/console debug:container --tag sulu_content.property_resolver
  • Get All resource loaders: bin/console debug:container --tag sulu_content.resource_loader

@Prokyonn Prokyonn force-pushed the feature/content-resolver branch from 3b54d40 to 62239fe Compare November 4, 2024 13:45
@alexander-schranz alexander-schranz changed the base branch from 2.6 to 3.0 November 8, 2024 15:07
@alexander-schranz
Copy link
Member

As discussed we will target the 3.0 for the new resolver and merge other bundles into that branch for easier maintainance, but that will be its own Pull requests.

@Prokyonn Prokyonn force-pushed the feature/content-resolver branch from bee9051 to cd473bd Compare November 17, 2024 14:15
@alexander-schranz
Copy link
Member

Moved TODO list into own issue: #7672

@alexander-schranz alexander-schranz marked this pull request as ready for review November 25, 2024 10:46
@alexander-schranz alexander-schranz merged commit 65825e7 into sulu:3.0 Nov 25, 2024
8 of 9 checks passed
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 Sulu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants