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

Fetch all available discussions in a single DB query for this post in the Renderer #33

Open
n-peugnet opened this issue Feb 18, 2023 · 0 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@n-peugnet
Copy link
Member

n-peugnet commented Feb 18, 2023

Currently the Renderer makes 1 DB query for each CROSSREFERENCE* tag in the post.

public function __invoke(Renderer $renderer, $context, ?string $xml, ?ServerRequestInterface $request)
{
if (is_null($request)) {
if ($this->config->inDebugMode()) {
$msg = 'request is "null", falling back to display discussion as for Guest. This is probably due to another extension not passing this parameter to "Formatter->render()". See stack trace below.';
$this->log->report(new RuntimeException($msg));
}
$actor = new Guest();
} else {
$actor = RequestUtil::getActor($request);
}
$filterCrossReferences = function ($attributes) use ($actor) {
/** @var Discussion|null */
$discussion = Discussion::whereVisibleTo($actor)->firstWhere('id', $attributes['id']);
if ($discussion) {
$attributes['title'] = $discussion->title;
} else {
$attributes['title'] = $this->translator->trans('club-1-cross-references.forum.unknown_discussion');
$attributes['unknown'] = true;
}
$attributes['comment'] = $this->translator->trans('club-1-cross-references.forum.comment');
return $attributes;
};
$xml = Utils::replaceAttributes($xml, 'CROSSREFERENCESHORT', $filterCrossReferences);
$xml = Utils::replaceAttributes($xml, 'CROSSREFERENCEURL', $filterCrossReferences);
$xml = Utils::replaceAttributes($xml, 'CROSSREFERENCEURLCOMMENT', $filterCrossReferences);
return $xml;
}
}

It is not very optimised for posts that contains multiple tags of this kind. Even less so when multiple tags all refer to the same discussion. It would be good first aggregate all the discussion ids to be able to fetch all available discussion in a single DB query.
Then, at replacement time, if the discussion is not in the fetched collection, when can mark it as unknown.

This is also a good opportunity to refactor the code to parse the XML only once and make the replacements in the resulting DOM instead of parsing it 3 times with Utils::replaceAttributes().

@n-peugnet n-peugnet added enhancement New feature or request good first issue Good for newcomers labels Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

1 participant