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

Whitelist global functions by default #231

Merged

Conversation

theofidry
Copy link
Member

@theofidry theofidry commented Jun 30, 2018

Closes #226

Besides the comment review, this is missing:

  • Docs: it is a BC break and also have some limitations (does it? - need to think about it)
  • Dedicated specs tests (is it necessary?)

.travis.yml Outdated
@@ -29,6 +29,7 @@ install:
- composer install --no-interaction --no-progress --no-suggest --prefer-dist $COMPOSER_FLAGS

script:
- ["$(ls _specs)" ] || echo "Empty"
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests that no files is in _specs. Indeed this directory should be used only for development but leaving files there is a mistake. It should however be extracted in a dedicated PR

scoper.inc.php Outdated
@@ -15,7 +15,6 @@
use Isolated\Symfony\Component\Finder\Finder;

return [
'prefix' => 'Foo',
'whitelist' => [
Copy link
Member Author

Choose a reason for hiding this comment

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

should be done in a different PR

@@ -287,7 +289,23 @@ private static function retrieveWhitelist(array $config): Whitelist
}
}

return Whitelist::create($whitelistGlobalConstants, ...$whitelist);
if (false === array_key_exists(self::WHITELIST_GLOBAL_FUNCTIONS_KEYWORD, $config)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

tests are missing for this new option

use function iterator_to_array;
use const PHP_EOL;
use PhpParser\Node\Name\FullyQualified;
use function sprintf;

final class ScoperAutoloadGenerator
Copy link
Member Author

Choose a reason for hiding this comment

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

tests are missing

use PhpParser\Node\Name\FullyQualified;
use function count;

final class UserGlobalFunctionCollection implements IteratorAggregate, Countable
Copy link
Member Author

Choose a reason for hiding this comment

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

tests are missing

use function count;

/**
* @TODO
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO

*
* @private
*/
final class FunctionIdentifierRecorder extends NodeVisitorAbstract
Copy link
Member Author

Choose a reason for hiding this comment

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

see if this class needs to be added all the time or if there is one time where it is not necessary (the less visitors the better performance wise)

@@ -36,8 +38,10 @@
private $constants;
Copy link
Member Author

Choose a reason for hiding this comment

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

missing tests

@@ -93,7 +93,7 @@ public function enterNode(Node $node): Node
private function shouldPrefixScalar(Node $node, bool &$isSpecialFunction): bool
{
if (false === ($node instanceof String_ && AppendParentNode::hasParent($node) && is_string($node->value))
|| 1 !== preg_match('/^((\\\\)?[\p{L}_]+)|((\\\\)?(?:[\p{L}_]+\\\\+)+[\p{L}_]+)$/u', $node->value)
|| 1 !== preg_match('/^((\\\\)?[\p{L}_]+)$|((\\\\)?(?:[\p{L}_]+\\\\+)+[\p{L}_]+)$/u', $node->value)
Copy link
Member Author

Choose a reason for hiding this comment

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

is a separate bug and should be fixed in a dedicated PR

@theofidry theofidry force-pushed the feature/whitelist-global-functions branch from 8a8736a to 60e768b Compare July 1, 2018 20:09
@theofidry theofidry force-pushed the feature/whitelist-global-functions branch from 2d19d01 to 1347997 Compare July 1, 2018 22:32
@theofidry theofidry changed the title [PoC] Whitelist global functions by default Whitelist global functions by default Jul 1, 2018
@theofidry theofidry merged commit 8049ea6 into humbug:master Jul 1, 2018
@theofidry theofidry deleted the feature/whitelist-global-functions branch July 1, 2018 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant