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.

Sign up for GitHub

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

Jump to bottom

Sync FileCacheStorage with changes from phpstan-src #498

Merged
merged 14 commits into from Jul 26, 2021
Merged

Sync FileCacheStorage with changes from phpstan-src #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ea36aec
perf optimize FileCacheStorage
clxmstaab Jul 23, 2021
fa3b02f
sync FIleCacheStorage impl with upstream phpstan
clxmstaab Jul 26, 2021
5edf306
update comment
clxmstaab Jul 26, 2021
8e848cc
Update FileCacheStorage.php
clxmstaab Jul 26, 2021
6823d1a
use Rector\Caching\ValueObject\CacheItem
clxmstaab Jul 26, 2021
e1d0fee
fix phpstan: use CachingException
clxmstaab Jul 26, 2021
0ccb973
fix phpstan: use smartFileSystem
clxmstaab Jul 26, 2021
1eb741b
re-added nearly everything again
clxmstaab Jul 26, 2021
97b01b6
Update FileCacheStorage.php
clxmstaab Jul 26, 2021
08f64a7
remove String::substring()
clxmstaab Jul 26, 2021
b0be077
Update FileCacheStorage.php
clxmstaab Jul 26, 2021
38da1ee
ignore phpstan errors
clxmstaab Jul 26, 2021
cd54f0c
fix path
clxmstaab Jul 26, 2021
3344e4a
Update phpstan.neon
clxmstaab Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 43 additions & 43 deletions packages/Caching/ValueObject/Storage/FileCacheStorage.php
View file
Open in desktop
Expand Up @@ -5,15 +5,14 @@
namespace Rector\Caching\ValueObject\Storage;

use Nette\Utils\Random;
use Nette\Utils\Strings;
use Rector\Caching\ValueObject\CacheFilePaths;
staabm marked this conversation as resolved.
Show resolved Hide resolved
use Rector\Caching\ValueObject\CacheItem;
use Symplify\EasyCodingStandard\Caching\Exception\CachingException;
use PHPStan\File\FileWriter;
use Symplify\SmartFileSystem\SmartFileSystem;
use Symplify\EasyCodingStandard\Caching\Exception\CachingException;

/**
* Inspired by
* https://github.com/phpstan/phpstan-src/commit/4df7342f3a0aaef4bcd85456dd20ca88d38dd90d#diff-6dc14f6222bf150e6840ca44a7126653052a1cedc6a149b4e5c1e1a2c80eacdc
* Inspired by https://github.com/phpstan/phpstan-src/blob/1e7ceae933f07e5a250b61ed94799e6c2ea8daa2/src/Cache/FileCacheStorage.php
*/
final class FileCacheStorage
{
Expand All @@ -24,59 +23,60 @@ public function __construct(
}

/**
* @param string $key
* @param string $variableKey
* @return mixed|null
*/
public function load(string $key, string $variableKey)
{
$cacheFilePaths = $this->getCacheFilePaths($key);

$filePath = $cacheFilePaths->getFilePath();
if (! is_file($filePath)) {
return null;
}

$cacheItem = require $filePath;
if (! $cacheItem instanceof CacheItem) {
return null;
}

if (! $cacheItem->isVariableKeyValid($variableKey)) {
return null;
}

return $cacheItem->getData();
return (function (string $key, string $variableKey) {
$cacheFilePaths = $this->getCacheFilePaths($key);

$filePath = $cacheFilePaths->getFilePath();
if (!\is_file($filePath)) {
return null;
}
$cacheItem = (require $filePath);
if (!$cacheItem instanceof CacheItem) {
return null;
}
if (!$cacheItem->isVariableKeyValid($variableKey)) {
return null;
}
return $cacheItem->getData();
})($key, $variableKey);
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 very confusing to read. Not sure if args are on left or right.
Could you extract function to assign line and invoke it on 2nd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its the way how phpstan does this things.

all these nitpicks are great, but it goes against intially mentioned goal:

Current design is based on cache in PHPStan, and I would like to keep it as close the original to make the maintenance easier.

after we apply all 'rector rules' the diff between the phpstan impl and ours here gets bigger and bigger.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, you're right. Let's give it a go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw, I opened a upstream PR to simplify this on phpstan-src

phpstan/phpstan-src#593

Copy link
Member

Choose a reason for hiding this comment

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

Cool! 👍

This is too much magic for me. I understand only objects and methods :D

}

/**
* @param string $key
* @param string $variableKey
* @param mixed $data
* @return void
Copy link
Member

Choose a reason for hiding this comment

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

It seems these duplicate the PHP code

*/
public function save(string $key, string $variableKey, $data): void
public function save(string $key, string $variableKey, $data) : void
{
$cacheFilePaths = $this->getCacheFilePaths($key);

$this->smartFileSystem->mkdir($cacheFilePaths->getFirstDirectory());
$this->smartFileSystem->mkdir($cacheFilePaths->getSecondDirectory());
$path = $cacheFilePaths->getFilePath();

$tmpPath = sprintf('%s/%s.tmp', $this->directory, Random::generate());
$errorBefore = error_get_last();
$exported = @var_export(new CacheItem($variableKey, $data), true);
$errorAfter = error_get_last();

$tmpPath = \sprintf('%s/%s.tmp', $this->directory, Random::generate());
$errorBefore = \error_get_last();
$exported = @\var_export(new CacheItem($variableKey, $data), true);
$errorAfter = \error_get_last();
if ($errorAfter !== null && $errorBefore !== $errorAfter) {
$errorMessage = sprintf(
'Error occurred while saving item "%s" ("%s") to cache: "%s"',
$key,
$variableKey,
$errorAfter['message']
);
throw new CachingException($errorMessage);
throw new CachingException(\sprintf('Error occurred while saving item %s (%s) to cache: %s', $key, $variableKey, $errorAfter['message']));
}
// for performance reasons we don't use SmartFileSystem
FileWriter::write($tmpPath, \sprintf("<?php declare(strict_types = 1);\n\nreturn %s;", $exported));
$renameSuccess = @\rename($tmpPath, $path);
if ($renameSuccess) {
return;
}
@\unlink($tmpPath);
if (\DIRECTORY_SEPARATOR === '/' || !\file_exists($path)) {
Copy link
Member

Choose a reason for hiding this comment

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

have you tried in different OS ? DIRECTORY_SEPARATOR is / in macOS:

➜  ~ php -a
Interactive shell

php > echo DIRECTORY_SEPARATOR;
/

php > var_dump(\DIRECTORY_SEPARATOR === '/');
bool(true)

Above code will make always throw exception.

Copy link
Contributor Author

@staabm staabm Jul 26, 2021 •
edited

Choose a reason for hiding this comment

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

this implementation is taken 1:1 from phpstan.

I guess this condition is meant as a non-windows feature detect.
As this class - as is - works in phpstan for months without further bugfixes, I feel this is good enough.

Copy link
Member

Choose a reason for hiding this comment

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

That possibly a dead code then, true || false will result true, true || true will result true, see https://3v4l.org/pKcY1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't tell the details why phpstan has implemented it like that.

Copy link
Member

Choose a reason for hiding this comment

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

Some comment would be useful so we don't use this blindly in the future

throw new CachingException(\sprintf('Could not write data to cache file %s.', $path));
}

$variableFileContent = sprintf("<?php declare(strict_types = 1);\n\nreturn %s;", $exported);
$this->smartFileSystem->dumpFile($tmpPath, $variableFileContent);

$this->smartFileSystem->rename($tmpPath, $cacheFilePaths->getFilePath(), true);
$this->smartFileSystem->remove($tmpPath);
}

public function clean(string $cacheKey): void
Expand All @@ -98,8 +98,8 @@ public function clear(): void
private function getCacheFilePaths(string $key): CacheFilePaths
{
$keyHash = sha1($key);
$firstDirectory = sprintf('%s/%s', $this->directory, Strings::substring($keyHash, 0, 2));
$secondDirectory = sprintf('%s/%s', $firstDirectory, Strings::substring($keyHash, 2, 2));
$firstDirectory = sprintf('%s/%s', $this->directory, substr($keyHash, 0, 2));
$secondDirectory = sprintf('%s/%s', $firstDirectory, substr($keyHash, 2, 2));
Copy link
Member

Choose a reason for hiding this comment

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

👍

$filePath = sprintf('%s/%s.php', $secondDirectory, $keyHash);

return new CacheFilePaths($firstDirectory, $secondDirectory, $filePath);
Expand Down
26 changes: 24 additions & 2 deletions phpstan.neon
View file
Open in desktop
Expand Up @@ -459,8 +459,6 @@ parameters:
message: '#Access to an undefined property PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocChildNode\:\:\$value#'
path: packages/BetterPhpDocParser/PhpDocInfo/PhpDocInfo.php #348

- '#"@var_export\(new \\Rector\\Caching\\ValueObject\\CacheItem\(\$variableKey, \$data\), true\)" is forbidden to use#'

-
message: '#Class with base "StaticTypeMapper" name is already used in "Rector\\PHPStanStaticTypeMapper\\TypeMapper\\StaticTypeMapper", "Rector\\StaticTypeMapper\\StaticTypeMapper"\. Use unique name to make classes easy to recognize#'
path: packages/StaticTypeMapper/StaticTypeMapper.php #31
Expand Down Expand Up @@ -528,3 +526,27 @@ parameters:
- '#Method "refactorParamType\(\)" returns bool type, so the name should start with is/has/was#'
- '#Method "decorateReturnWithSpecificType\(\)" returns bool type, so the name should start with is/has/was#'
- '#Method "resolveObjectType\(\)" returns bool type, so the name should start with is/has/was#'

# resolve later
-
message: '#Use explicit names over dynamic ones#'
TomasVotruba marked this conversation as resolved.
Show resolved Hide resolved
paths:
- packages/Caching/ValueObject/Storage/FileCacheStorage.php

# native filesystem calls, required for performance reasons
-
message: '#"@\\unlink\(\$tmpPath\)" is forbidden to use#'
paths:
- packages/Caching/ValueObject/Storage/FileCacheStorage.php
-
message: '#"@\\rename\(\$tmpPath, \$path\)" is forbidden to use#'
paths:
- packages/Caching/ValueObject/Storage/FileCacheStorage.php
-
message: '#"%s" in sprintf\(\) format must be quoted#'
paths:
- packages/Caching/ValueObject/Storage/FileCacheStorage.php
-
message: '#"@\\var_export\(new \\Rector\\Caching\\ValueObject\\CacheItem\(\$variableKey, \$data\), true\)" is forbidden to use#'
paths:
- packages/Caching/ValueObject/Storage/FileCacheStorage.php