Multiple assertions develop #379

basz · 2018-02-18T20:56:03Z

Implements #320 for develop branch

does so without performance loss if not used
assertion sets may be nested with different conditions
does so lazily for each condition
- an AND condition will break early as soon as one assertions fails
- an OR condition will break early as soon as one assertions succeed
assertions are cached, so a next call won't require to construct an assertion or AssertionSet.

documentation forthcoming

…ion classes

basz · 2018-02-18T21:05:42Z

no sure why this PR also show commits already pulled into develop.

review request is from commit e7fd215 and forward

svycka

Also, I'm not sure why unrelated changes were added for example CS check and Travis config to this PR. Also maybe we can require php7.1 and we could use void return type as we now use it. I don't know why so many unrelated changes are in this PR maybe that's what you said that some commits from develop branch is here. but then maybe rebase is required?

svycka · 2018-02-19T07:04:12Z

src/Container/AuthorizationServiceFactory.php

     * @return AuthorizationService
     */
-    public function createService(ServiceLocatorInterface $serviceLocator)
+    public function createService(ServiceLocatorInterface $serviceLocator): AuthorizationService


this is for ServiceManager v2 and we no longer support it so can be removed

svycka · 2018-02-19T07:06:38Z

src/Container/ModuleOptionsFactory.php

     * @return ModuleOptions
     */
-    public function createService(ServiceLocatorInterface $serviceLocator)
+    public function createService(ServiceLocatorInterface $serviceLocator): ModuleOptions


also not used anymore and in other factories also...

svycka · 2018-02-19T07:08:20Z

src/Container/ObjectRepositoryRoleProviderFactory.php

@@ -40,15 +42,15 @@ class ObjectRepositoryRoleProviderFactory implements FactoryInterface
    protected $options = [];

    /**
-     * {@inheritDoc}
+     * {@inheritdoc}
     */
    public function __construct(array $options = [])


do we need this constructor?

svycka · 2018-02-19T07:09:46Z

src/Container/ObjectRepositoryRoleProviderFactory.php

     */
    public function __construct(array $options = [])
    {
        $this->setCreationOptions($options);
    }

    /**
-     * {@inheritDoc}
+     * {@inheritdoc}
     */
    public function setCreationOptions(array $options)


$options from __invoke(ContainerInterface $container, $requestedName, array $options = null) method can be used instead this method also from SM v2

svycka · 2018-02-19T07:11:26Z

src/Container/RoleServiceFactory.php

     * @return RoleService
     */
-    public function createService(ServiceLocatorInterface $serviceLocator)
+    public function createService(ServiceLocatorInterface $serviceLocator): RoleService


svycka · 2018-02-19T07:31:49Z

src/Role/Role.php

@@ -1,4 +1,6 @@
 <?php


I don't like that git show that this file was renamed test/Asset/Permission.php → src/Role/Role.php but not sure if that's important :D

I remember there is some git plugin that fixes that, but I forgot the name.

svycka · 2018-02-19T07:33:47Z

src/Role/RoleProviderPluginManager.php

    /**
     * @var array
     */
-    protected $invokableClasses = [
-        InMemoryRoleProvider::class => InMemoryRoleProvider::class
+    protected $aliases = [


do we really need those aliases? I think FCQN is what everyone uses.

svycka · 2018-02-19T07:47:56Z

test/Asset/MockRoleWithPermissionMethod.php

    {
        return 'role-with-permission-method';
    }
-    public function hasPermission($permission)
+
+    public function hasPermission(string $permission): bool
    {
        return false;


shouldn't this be something like return in_array($permission, $this->getPermissions(), true);? This may is test asset but can lead to problems later if someone will assume that this is valid role

If someone has a problem when using a MockRole from TestAsset in production, he deserves this problem :)

svycka · 2018-02-19T07:56:00Z

test/Options/ModuleOptionsTest.php

@@ -36,10 +40,10 @@ public function testAssertModuleDefaultOptions()
    public function testSettersAndGetters()
    {
        $moduleOptions = new ModuleOptions([
-            'guest_role'            => 'unknown',
-            'role_provider'         => [],
+            'guest_role'               => 'unknown',


I don't like those spaces but i guess that's not for me to decide :D

svycka · 2018-02-19T08:02:25Z

src/Role/HierarchicalRole.php

+/**
+ * Simple implementation for a hierarchical role
+ */
+final class HierarchicalRole extends Role implements HierarchicalRoleInterface


maybe if we use final then add to others also?

agree, when it's implementing an interface and has no additional public methods, let's mark as final

DavidHavl · 2018-02-19T11:06:08Z

Looks great!!

basz · 2018-02-19T12:07:37Z

@svycka thanks. yes, I will do additional changes like php71, remove zf2 support etc... for now could you have a look at the changes from commit e7fd215 and forward?

svycka · 2018-02-19T12:53:49Z

@svycka thanks. yes, I will do additional changes like php71, remove zf2 support etc... for now could you have a look at the changes from commit e7fd215 and forward?

@basz I already did, but if you could rebase would be a lot easier to review because now it is very hard to see relevant changes.

basz · 2018-02-19T13:06:49Z

I now figured out what goed wrong. this PR #366 isn't merged into develop yet. Guess I should fix that one first and then rebase this one.

prolic · 2018-02-20T16:06:52Z

composer.json

-    "squizlabs/php_codesniffer": "^2.0",
-    "doctrine/common": "~2.4"
+    "malukenho/docheader": "^0.1",
+    "phpunit/phpunit": "^6.0",


let's use v7

prolic · 2018-02-20T16:07:32Z

src/Assertion/AssertionSet.php

+use ZfcRbac\Exception;
+use ZfcRbac\Identity\IdentityInterface;
+
+class AssertionSet implements AssertionInterface


if it's implementing an interface, mark as final

prolic · 2018-02-20T16:08:45Z

src/Role/HierarchicalRole.php

+/**
+ * Simple implementation for a hierarchical role
+ */
+final class HierarchicalRole extends Role implements HierarchicalRoleInterface


agree, when it's implementing an interface and has no additional public methods, let's mark as final

prolic · 2018-02-20T16:09:36Z

src/Role/Role.php

@@ -1,4 +1,6 @@
 <?php


I remember there is some git plugin that fixes that, but I forgot the name.

prolic · 2018-02-20T16:09:46Z

src/Role/Role.php

 */
-class Permission implements PermissionInterface
+class Role implements RoleInterface


same as above, if it's implementing an interface, mark as final

prolic · 2018-02-20T16:10:28Z

test/Assertion/AssertionSetTest.php

+ */
+class AssertionSetTest extends TestCase
+{
+    public function testImplementsAssertionInterface()


do we require php 7.1 now? if so, let's add : void

prolic · 2018-02-20T16:11:30Z

test/Asset/MockRoleWithPermissionMethod.php

    {
        return 'role-with-permission-method';
    }
-    public function hasPermission($permission)
+
+    public function hasPermission(string $permission): bool
    {
        return false;


If someone has a problem when using a MockRole from TestAsset in production, he deserves this problem :)

prolic · 2018-02-20T16:12:04Z

Looks good so far, just some minor notes

basz added 21 commits April 5, 2017 08:56

update dependencies

d1e0751

update test to work with phpunit 6

26d7d46

patch test case without tests

5ffbad2

php7 return types

f432acc

copy zfr-rbac into repository

1e8a77f

adding docheader to pipline

f9841b0

switch notifications

b2dd14f

adding return type to ZfcRbac/Rbac which results in removal of Except…

868083b

…ion classes

fix travis doccheck

8b9d261

review feedback

40b3082

PermissionInterface is gone

bb74310

adds a test

918b1d6

remove php56 from travis matrix

4902b38

switch cs tooling updated travis conf

ef3bb59

apply code styling

a27a554

ZfcRbac\Rbac\Role to ZfcRbac\Role

881f839

support only sm3 plugin managers

b4287d1

v3 factories

dfe3c85

tests for sm3 plugin managers

43e1c95

typo and env addition

4f61a49

remove :void (for now)

5019c5e

basz requested a review from prolic February 18, 2018 20:56

basz mentioned this pull request Feb 18, 2018

Future #367

Open

svycka suggested changes Feb 19, 2018

View reviewed changes

basz force-pushed the multiple-assertions-develop branch from 29dc027 to 8c3d6a4 Compare February 19, 2018 12:58

basz added 16 commits February 20, 2018 12:48

remove hhvm

fe378ff

by popular demand no more spaces

242101d

optimize yielding

b6f3de5

params and doc block not needed

f69ed6d

unused property

dd0fb93

more unneeded doc block removed

f254ad5

stdlib ^3.1

92d46d6

remove allow failure from travis

875090b

revert unpack, does not work

bf40881

fix, must be of the type array, null given,

523fd25

cs

b1c1d1e

ternary assignment

2d10256

import class

9a4a615

ternary for real

cc27436

not needed as we switched to psr/container

66fedba

void return types on tests methods

2f89286

prolic reviewed Feb 20, 2018

View reviewed changes

prolic suggested changes Feb 20, 2018

View reviewed changes

prolic mentioned this pull request Feb 20, 2018

Develop #366

Merged

basz added 5 commits February 20, 2018 17:25

cache assertions

869abfb

adds assertion set

5c0290b

tests

10e6bcd

apply in AuthorizationService

02757b3

rebase conflict

81178b8

basz force-pushed the multiple-assertions-develop branch from 8c3d6a4 to 81178b8 Compare February 20, 2018 16:29

cs

175b48b

prolic merged commit c0bf5cb into ZF-Commons:develop Feb 20, 2018

basz mentioned this pull request Feb 22, 2018

Added ability to set multiple assertions and their condition for permissions #320

Open

basz deleted the multiple-assertions-develop branch February 22, 2018 11:51

Multiple assertions develop #379

Multiple assertions develop #379

Uh oh!

Conversation

basz commented Feb 18, 2018

Uh oh!

basz commented Feb 18, 2018

Uh oh!

svycka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidHavl commented Feb 19, 2018

Uh oh!

basz commented Feb 19, 2018

Uh oh!

svycka commented Feb 19, 2018

Uh oh!

basz commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prolic commented Feb 20, 2018

Uh oh!

Uh oh!