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

EZP-31281: Decoupled RoleDomainMapper from Repository #42

Merged
merged 3 commits into from Apr 10, 2020

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Apr 8, 2020

Question Answer
JIRA issue EZP-31281
Type improvement (aiming to fix a bug)
Target eZ Platform version v3.0.1
BC breaks no, internal
Tests pass yes
Doc needed no

This is one of the last steps towards decoupling PermissionResolver (cc @Nattfarinn)

I've moved \eZ\Publish\Core\Repository\Helper\RoleDomainMapper to \eZ\Publish\Core\Repository\Mapper\RoleDomainMapper.

For now I've kept Helper\RoleDomainMapper for temporary BC with 1st party packages to avoid fighting with false negative CI issues (used by ezplatform-http-cache Behat tests). To be dropped after merging this one and aligning ezplatform-http-cache.

After I was done with that, I've decoupled RoleDomainMapper from Repository. Merged into this PR to speed up process for #44. This is still the scope of EZP-31281 and makes it now complete.

Checklist:

  • PR description is updated.
  • Tests are passing.
  • Added code follows Coding Standards (use $ composer fix-cs).
  • PR is ready for a review.

@alongosz alongosz added the Improvement Changes not fixing or changing behavior label Apr 8, 2020
Copy link
Contributor

@mikadamczyk mikadamczyk left a comment

Choose a reason for hiding this comment

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

we could change MapperRoleDomainMapper to BaseRoleDomainMapper

Moved \eZ\Publish\Core\Repository\Helper\RoleDomainMapper to \eZ\Publish\Core\Repository\Mapper\RoleDomainMapper.

Kept Helper\RoleDomainMapper for temporary BC with 1st party packages.
@alongosz alongosz changed the title EZP-31281: Moved RoleDomainMapper from Helper to Mapper namespace EZP-31281: Decoupled RoleDomainMapper from Repository Apr 9, 2020
Copy link
Member Author

@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.

we could change MapperRoleDomainMapper to BaseRoleDomainMapper

Squashed into 0fa9110.

@mikadamczyk @adamwojs I've expanded the scope here by introducing decoupling as well, to speed up things for @Nattfarinn in #44.
Added b184253 and e51c154. Sorry for the inconvenience.

@@ -1075,34 +1074,16 @@ protected function getUserReferenceMock()
/**
* @return \eZ\Publish\API\Repository\Repository|\PHPUnit\Framework\MockObject\MockObject
*/
protected function getRepositoryMock($methods = [])
protected function getRepositoryMock(): Repository
Copy link
Member Author

@alongosz alongosz Apr 9, 2020

Choose a reason for hiding this comment

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

This was quite misleading as it was not used. Moreover inherited from parent Base which did not have this argument.
There were no usages with this argument in the codebase anyway.

{
if ($this->repositoryMock === null) {
$this->repositoryMock = $this
->getMockBuilder(CoreRepository::class)
->setMethods(['getPermissionResolver'])
->onlyMethods(['getPermissionResolver'])
Copy link
Member Author

Choose a reason for hiding this comment

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

setMethods is deprecated.

Comment on lines +347 to +360
protected function getRoleDomainMapperMock(array $methods = []): RoleDomainMapper
{
if ($this->roleDomainMapperMock === null) {
$mockBuilder = $this->getMockBuilder(RoleDomainMapper::class);
if (!empty($methods)) {
$mockBuilder->onlyMethods($methods);
}
$this->roleDomainMapperMock = $mockBuilder
->disableOriginalConstructor()
->getMock();
}

return $this->roleDomainMapperMock;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from child class as now it's needed here as well. Not 1:1 because setMethods is deprecated. Diff could be a bit confusing, because passing $methods directly will lead to "no methods" with an empty array, rather than what we've expected here.

@alongosz alongosz added the Ready for MERGE To be set by author or maintainer label Apr 10, 2020
@alongosz
Copy link
Member Author

Regression tests passed (see the last link to the private repo). Merging to unblock #44.

@alongosz alongosz merged commit 8997e2b into ezsystems:master Apr 10, 2020
@alongosz alongosz deleted the move-role-domain-mapper branch April 10, 2020 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Changes not fixing or changing behavior Ready for MERGE To be set by author or maintainer Ready for review
3 participants