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

Allow cleaning namespace only #52

Closed
wants to merge 11 commits into from
Closed

Allow cleaning namespace only #52

wants to merge 11 commits into from

Conversation

aiphee
Copy link

@aiphee aiphee commented Aug 4, 2017

Added option to clean only selected workspace. Repaired comments in changed files.

@aiphee aiphee changed the title Přidání mazání namespace Allow cleaning namespace only Aug 4, 2017
@dg
Copy link
Member

dg commented Aug 5, 2017

You changed some tabs to spaces, can you fix it and repush?

@aiphee
Copy link
Author

aiphee commented Aug 5, 2017

Yeah sorry, didn't notice in my editor.

@dg
Copy link
Member

dg commented Aug 5, 2017

There are still a lot of modifications in phpDoc which are not needed.

@aiphee
Copy link
Author

aiphee commented Aug 5, 2017

OK, i will delete that later. I usually do that because phpStorm hints are broken when phpDoc is off, but i guess that is for different pull request anyway.

@aiphee
Copy link
Author

aiphee commented Aug 5, 2017

OK, comments are back as they were and i added test, i wasn't sure with SQLite test, so i didn't do that one yet.

PS: I had some problems with testing, is there some way to download whole nette package with tests?

}

foreach ($namespaces as $namespace) {
$dir = $this->dir . "/_$namespace";
Copy link
Member

Choose a reason for hiding this comment

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

It should be $dir = $this->dir . '/_' . urlencode($namespace);

foreach ($namespaces as $namespace) {
$dir = $this->dir . "/_$namespace";
if (is_dir($dir)) {
$items = Nette\Utils\Finder::findFiles('')->from($dir);
Copy link
Member

Choose a reason for hiding this comment

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

findFiles('*') seems more readable

foreach ($items as $item) {
$this->delete((string) $item);
}
@rmdir($dir);
Copy link
Member

Choose a reason for hiding this comment

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

Please add comment // may already contain new files

@@ -249,6 +249,7 @@ public function clean(array $conditions): void
{
$all = !empty($conditions[Cache::ALL]);
$collector = empty($conditions);
$namespaces = $conditions[Cache::NAMESPACE] ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

You can convert it to array immediately, for example $namespace = (array) $conditions[Cache::NAMESPACE] ?? null;

Copy link
Member

Choose a reason for hiding this comment

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

Btw, probably better name is NAMESPACES to be consistent with TAGS and FILES

Copy link
Author

Choose a reason for hiding this comment

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

Not sure how you mean it with retyping it to array at top. It wouldn't work even if it were enclosed in brackets.

}

foreach ($namespaces as $namespace) {
$this->pdo->prepare("DELETE FROM cache WHERE key LIKE %/_$namespace/%")->execute();
Copy link
Member

Choose a reason for hiding this comment

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

This is sql injection, it should be

$this->pdo->prepare('DELETE FROM cache WHERE key LIKE ?)')->execute([$namespace . Cache::NAMESPACE_SEPARATOR])

@dg
Copy link
Member

dg commented Aug 6, 2017

Good work!

When you install it via Composer composer create-project nette/caching, you can run tests via vendor/bin/tester.

@dg
Copy link
Member

dg commented Aug 18, 2017

Merged with a small modification, thanks!

dg pushed a commit that referenced this pull request Aug 18, 2017
dg pushed a commit that referenced this pull request Aug 18, 2017
dg pushed a commit that referenced this pull request Aug 18, 2017
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.

None yet

2 participants