[Security] [Acl] [MaskBuilder] Refactor common code and reduce nesting #9723

djlambert · 2013-12-08T16:22:34Z

jakzal · 2013-12-08T16:53:13Z

src/Symfony/Component/Security/Acl/Permission/MaskBuilder.php

+        }
+
+        return $mask;
+    }


validateMask() doesn't say the method returns anything. It implies value is validated but it's not clear it's also returned. I'd either merge it with getCodeMask() or rename it (findMask() ?).

@throws should go after @return.

djlambert · 2013-12-08T17:12:05Z

jakzal · 2013-12-08T17:14:10Z

src/Symfony/Component/Security/Acl/Permission/MaskBuilder.php

+        if (is_string($code)) {
+            if (!defined($name = sprintf('static::MASK_%s', strtoupper($code)))) {
+                throw new \InvalidArgumentException(sprintf('The code "%s" is not supported', $code));
+            }


Can we merge these two nested if statements into one? This is how it used to be before extraction.

I can, though it seemed confusing to me passing a bad string value and getting an exception saying it must be an integer. Changing the remaining exception's message to '$code must be a valid code or integer.' would remedy that somewhat.

@insekticid

jakzal reviewed Dec 8, 2 8000 013
View reviewed changes

Refactor common code and reduce nesting

7d85809

jakzal reviewed Dec 8, 2013
View reviewed changes

fabpot merged commit 7d85809 into symfony:master Dec 17, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Security] [Acl] [MaskBuilder] Refactor common code and reduce nesting #9723

[Security] [Acl] [MaskBuilder] Refactor common code and reduce nesting #9723

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Uh oh!

[Security] [Acl] [MaskBuilder] Refactor common code and reduce nesting #9723

[Security] [Acl] [MaskBuilder] Refactor common code and reduce nesting #9723

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!