-
Notifications
You must be signed in to change notification settings - Fork 19
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 DimensionContentQueryEnhancer service #190
Add DimensionContentQueryEnhancer service #190
Conversation
0ff0dca
to
2c5dc16
Compare
26613be
to
289d3d4
Compare
Pull Request Test Coverage Report for Build 1294915754
💛 - Coveralls |
Content/Infrastructure/Doctrine/DimensionContentQueryEnhancer.php
Outdated
Show resolved
Hide resolved
* @param class-string<DimensionContentInterface> $dimensionContentClassName | ||
* @param mixed[] $dimensionAttributes | ||
*/ | ||
public function enhanceDimensionContentSelect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you use this function in the SuluArticleBundle
?
if not, i think i would keep the logic in the DimensionContentRepository
for now.
if yes, i think i would call it addSelect
/addEffectiveDimensionContentSelect
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be used in the article bundle, but think I need first implement it here in the Example Bundle to also write tests for it.
ec5b56e
to
9c07160
Compare
Content/Infrastructure/Doctrine/DimensionContentQueryEnhancer.php
Outdated
Show resolved
Hide resolved
): void { | ||
$effectiveAttributes = $dimensionContentClassName::getEffectiveDimensionAttributes($dimensionAttributes); | ||
$this->addSortBy($queryBuilder, $effectiveAttributes); | ||
$queryBuilder->addCriteria($this->getAttributesCriteria('dimensionContent', $effectiveAttributes)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should pass a dimensionContentAlias
as parameter? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many other joins so I would not introduce a parameter for the aliases here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i would add a ALIAS_DIMENSION_CONTENT
constant then? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not yet add any constant here for aliases.
$examples = $queryBuilder->getQuery()->getResult(); | ||
|
||
foreach ($examples as $example) { | ||
yield $example; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an advantage of yielding the results here instead of returning an array? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method should be refractored to us toIterator
method of doctrine just need to test it. The advantage is a performance as if correctly used only 1 object instance is in memory why looping over it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently just used a custom Generator to make sure all works with a Generator.
9c07160
to
2e8fc42
Compare
6a10002
to
7df8905
Compare
c69b7f5
to
a820d3f
Compare
65a8c70
to
5e0867b
Compare
@nnatter ready for review. |
} | ||
|
||
$effectiveAttributes = $dimensionContentClassName::getEffectiveDimensionAttributes($dimensionAttributes); | ||
$this->addSortBy($queryBuilder, $effectiveAttributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure if i would expect this method to change my sorting. you want to sort this to return the most specific DimensionContent
first, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also means that calling the addSelects
method will lead to multiple rows per entity, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove this sorting. in the future maybe it make sense that we can sort specially for smart content by published date but that should be handled in a additional addSortBy method.
Content/Infrastructure/Doctrine/DimensionContentQueryEnhancer.php
Outdated
Show resolved
Hide resolved
); | ||
} | ||
|
||
$locale = $dimensionAttributes['locale'] ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think $dimensionAttributes
is not defined anywhere? wondering why phpstan is okay with this? 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is false, funny that the test doesn't cover this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did update it.
|
||
$contentFilters = []; | ||
foreach ([ | ||
'locale', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be needed in every repository and makes it harder to extend things 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can just pass all $filters
to the dimensionContentQueryEnhancer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we could add a prefix for filters that should be passed to the dimensionContentQueryEnhancer
? eg dimensionContent.categoryIds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine currently to forward the filters directly to the enhancer
'dimensionContent' | ||
); | ||
|
||
$this->dimensionContentQueryEnhancer->addSelects( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will lead to multiple rows for a single example like described in #190 (comment), right? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed addSelect will just add the joined to relation and not create a new instance of the example object.
94bb509
to
dd1a227
Compare
/** | ||
* @var DimensionContentInterface|null | ||
*/ | ||
private $unlocalizedDimensionContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this are private variables which were just set but not used.
dd1a227
to
15c63ee
Compare
15c63ee
to
d9497bd
Compare
@nnatter Ready for rereview ;) |
The DimensionContentQueryEnhancer will make it possible the Repository of a ContentRichEntity in our current case the Article. The DimensionContentQueryEnhancer provide all required filters like categories, tags, template keys which are used by smart content. Also it provide the logic how the select join work as they are very complex to select only the dimensions which are required to be performant. The current implementation is very imperformant as when a list is loaded all dimension content are loaded as separate queries and are not correctly joined.