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-5121: Streamlined SiteaccessResolverInterface #722

Merged
merged 5 commits into from
Apr 6, 2023

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Mar 2, 2023

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-5121
Bug fix? no
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no(?)
License GPL-2.0

As a part of changes, I moved \Ibexa\PageBuilder\Siteaccess\SiteaccessService::resolveSiteAccessForContent into \Ibexa\AdminUi\Siteaccess\SiteaccessResolverInterface as it seems that is a proper place for it. As additional cleanup, I had deprecated \Ibexa\AdminUi\Siteaccess\SiteaccessResolverInterface::getSiteaccessesForLocation as getSiteAccessesListForLocation does basically the same thing, and the naming is quite confusing - same thing was done for getSiteaccesses method.

Checklist:

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

@ViniTou ViniTou force-pushed the ibx-5121-streamlined-sa-resolver-interface branch from bb94ae7 to 091fb94 Compare March 2, 2023 10:53
@ViniTou ViniTou requested a review from a team March 16, 2023 11:45
src/lib/Siteaccess/NonAdminSiteaccessResolver.php Outdated Show resolved Hide resolved
src/lib/Siteaccess/NonAdminSiteaccessResolver.php Outdated Show resolved Hide resolved
'name'
);
}

public function getSiteAccessesList(): array
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: phpdoc could be more specific about type of returned value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is in interface.

@ViniTou ViniTou requested review from Steveb-p, adamwojs and a team March 27, 2023 15:45
@ViniTou ViniTou force-pushed the ibx-5121-streamlined-sa-resolver-interface branch from d05fba1 to cbac63c Compare March 28, 2023 11:38
@ViniTou ViniTou force-pushed the ibx-5121-streamlined-sa-resolver-interface branch from cbac63c to c50bd39 Compare April 4, 2023 13:19
@sonarcloud
Copy link

sonarcloud bot commented Apr 4, 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

Copy link
Contributor

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on IbexaDXP commerce 4.5

@ViniTou ViniTou merged commit 15eee83 into main Apr 6, 2023
@ViniTou ViniTou deleted the ibx-5121-streamlined-sa-resolver-interface branch April 6, 2023 08:28
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.

None yet

7 participants