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

[SD-360] Refactor ::getHierarchy #543

Merged

Conversation

vincent-gao
Copy link
Contributor

@vincent-gao vincent-gao commented Nov 21, 2024

Jira

https://digital-vic.atlassian.net/browse/SD-360

Problem/Motivation

cache ::buildHierarchy results. since ::buildHierarchy consumes a significant amount of resources, it's surprising that the results are not being cached~

@vincent-gao vincent-gao marked this pull request as ready for review November 21, 2024 12:22
@vincent-gao vincent-gao self-assigned this Nov 21, 2024
@vincent-gao vincent-gao force-pushed the SD-360-refactor-getHierarchy-for-tide_publication branch from 93bac1e to 0f81655 Compare November 25, 2024 06:52
@@ -181,53 +182,110 @@ public function getHierarchy(EntityInterface $entity, Request $request) {
* The hierarchy.
*/
protected function buildHierarchy(ContentEntityInterface $entity, NestedSetStorage $storage, CacheableMetadata $cache, $weight, array &$flatten_hierarchy, $site = NULL) {
$resource_type = $this->resourceTypeRepository->get($entity->getEntityTypeId(), $entity->bundle());
// Try cache first.
$cid = "tide_publication:hierarchy:{$entity->uuid()}:{$weight}:" . ($site ? $site->id() : 'null');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these get surrounded by {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @anthony-malkoun , it just clearly defines variable boundary in this string.

$hierarchy = [
// Cache the results.
$cache_data = [
'hierarchy' => $hierarchy,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to save both here? You only use flatten?

Copy link
Contributor Author

@vincent-gao vincent-gao Nov 25, 2024

Choose a reason for hiding this comment

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

This refactor confused me a bit at first! I wanted to build a hierarchical tree structure while also maintaining a flattened array via the reference parameter 😇 . Anyway, I’ve removed the unnecessary calculation results.
Thanks for catching this issue!

$cid = "tide_publication:hierarchy:{$entity->uuid()}:{$weight}:" . ($site ? $site->id() : 'null');
if ($cached = $this->cacheData->get($cid)) {
$cache->addCacheTags($cached->tags);
$flatten_hierarchy = array_merge($flatten_hierarchy, $cached->data['flatten']);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get this? Isn't flatten_hierarchy derived from the node?

Copy link
Contributor

@anthony-malkoun anthony-malkoun left a comment

Choose a reason for hiding this comment

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

I think most of my comments are just questions, otherwise this looks fine and good to me.

@vincent-gao
Copy link
Contributor Author

hey @anthony-malkoun
Thanks for the code review, I have removed unnecessary code!

@vincent-gao vincent-gao merged commit bd2415c into develop Nov 25, 2024
1 check failed
@vincent-gao vincent-gao deleted the SD-360-refactor-getHierarchy-for-tide_publication branch November 25, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants