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

IBX-6589: Added optional parameter siteAccessList to SiteaccessResolver #910

Open
wants to merge 1 commit into
base: 4.5
Choose a base branch
from

Conversation

ciastektk
Copy link
Contributor

@ciastektk ciastektk commented Sep 19, 2023

Question Answer
Tickets IBX-6589
Bug fix? no
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

This PR adds optional parameter siteAccessList to SiteaccessResolver::getSiteAccessesListForContent and SiteaccessResolver::getSiteAccessesListForLocation methods, to only iterate over the passed SiteAccesses list.
This will improve performance when iterating over all SiteAccesses are not needed.

Checklist:

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

@sonarcloud
Copy link

sonarcloud bot commented Sep 19, 2023

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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ciastektk ciastektk marked this pull request as ready for review September 19, 2023 09:20
@ciastektk ciastektk requested a review from a team September 19, 2023 09:20
Copy link
Contributor

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

Couldnt SiteAccessService::getAll() method result be cached instead of passing this for multiple layers?
Or maybe the list to iterate should be set in a constructor if for some reason this needs to vary from checking on all siteaccesses.

Also, could you show how much we gain by this if this is performance related?

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.

4 participants