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-31480: Introduced strict types for PermissionResolver & PermissionCriterionResolver #2986

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Mar 13, 2020

Question Answer
JIRA issue EZP-31480
Bug/Improvement yes
New feature no
Target version master
BC breaks yes
Tests pass yes
Doc needed yes

This PR introduces strict types for the \eZ\Publish\API\Repository\\eZ\Publish\API\Repository\PermissionResolver and \eZ\Publish\API\Repository\PermissionCriterionResolver

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

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.

One request, if no time now, maybe as a follow-up?

@@ -25,5 +27,5 @@ interface PermissionCriterionResolver
*
* @return bool|\eZ\Publish\API\Repository\Values\Content\Query\Criterion
*/
public function getPermissionsCriterion($module, $function, ?array $targets = null);
public function getPermissionsCriterion(string $module = 'content', string $function = 'read', ?array $targets = null);
Copy link
Member

@alongosz alongosz Mar 16, 2020

Choose a reason for hiding this comment

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

Can we use this opportunity to drop default arguments here?

Reasoning: I honestly would have never guessed that

$permissionCriterionResolver->getPermissionsCriterion();

would give me Criteria for content/read - this is one of many policies.

Copy link
Member Author

@adamwojs adamwojs Mar 16, 2020

Choose a reason for hiding this comment

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

Definitely! @mikadamczyk requested this as well on Slack

@adamwojs adamwojs merged commit db13e33 into master Mar 16, 2020
@adamwojs adamwojs deleted the ezp_31480 branch March 16, 2020 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants