Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

IBX-6017: Avoid loading whole content in the Content Tree #2104

Merged
merged 3 commits into from
Jul 31, 2023

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Jun 22, 2023

Question Answer
Tickets IBX-6017
Improvement? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

The idea is that we could implement loadVersionInfos PAPI method and get translated name based on this, just like it's done in loadContentListByContentInfo - this would have a much higher impact on performance though.

Blackfire profiles are included in the ticket's comments.

Requires: ezsystems/ezplatform-kernel#375

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@webhdx
Copy link
Contributor

webhdx commented Jun 23, 2023

BC is kept as long as we still return any name. I vaguely remember why it was done this way but my guess would be that it was required to fetch Content name in current siteaccess prioritized language. With your change you likely return the name in Content's main language. We need to make sure there is no regression here.

@barw4
Copy link
Member Author

barw4 commented Jun 23, 2023

BC is kept as long as we still return any name. I vaguely remember why it was done this way but my guess would be that it was required to fetch Content name in current siteaccess prioritized language. With your change you likely return the name in Content's main language. We need to make sure there is no regression here.

@webhdx yes, this is exactly the case, we are prioritizing the first language defined in the current site access. And with the change following I'm returning the name in the main language of content, as you described.

@webhdx
Copy link
Contributor

webhdx commented Jun 23, 2023

We are talking about regression then. I'd say this improvement in current form is a no go as another customer will report that all of sudden they are not getting translated names. A feature toggle is also a flawed idea because you get performance gain at the cost of missing a feature.

I'd rather explore the possibility of loading VersionInfo list as you described. But I personally see implementing node caching would work the best here but we are talking about larger feature than just a quick fix.

@barw4 barw4 changed the title [POC] IBX-6017: Avoid loading whole content in the Content Tree IBX-6017: Avoid loading whole content in the Content Tree Jun 28, 2023
@barw4 barw4 assigned barw4 and unassigned mikadamczyk, Nattfarinn and barw4 Jun 28, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@barw4
Copy link
Member Author

barw4 commented Jun 29, 2023

@webhdx related PR is ready with versionInfoList as discussed ezsystems/ezplatform-kernel#375

@webhdx webhdx requested a review from a team July 18, 2023 10:42
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

I'm assuming behat failure is related to missing kernel dependency :)

@alongosz alongosz requested a review from a team July 18, 2023 10:55
@barw4
Copy link
Member Author

barw4 commented Jul 18, 2023

I'm assuming behat failure is related to missing kernel dependency :)

@alongosz, yes, most errors are coming from the missing method (loadVersionInfoListByContentInfo) that was introduced in the kernel package.

@micszo micszo self-assigned this Jul 24, 2023
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

@micszo micszo removed their assignment Jul 31, 2023
@mikadamczyk mikadamczyk merged commit 6a8d87c into 2.3 Jul 31, 2023
@mikadamczyk mikadamczyk deleted the ibx-6017-content-tree-optimization branch July 31, 2023 13:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Development

Successfully merging this pull request may close these issues.

7 participants