[DX] Merge RectorConfig cacheDirectory() and containerCacheDirectory() to one to ease changing paths in case of non-writeable cache #4803

TomasVotruba · 2023-08-17T06:49:50Z

~~The Symfony container was compiled, but Laravel one is lazy, so doesn't use any kind of dumped cache file. PHPStan handles this in its own way, so we can drop it completely.~~

EDIT: This option is needed for PHPStan container. Yet should be united with other cache directory option to avoid duplicated work and WTFs in rectorphp/rector#8112

Closes: rectorphp/rector#8112

samsonasik · 2023-08-17T07:12:54Z

packages/NodeTypeResolver/DependencyInjection/PHPStanServicesFactory.php

@@ -49,7 +49,7 @@ public function __construct(

        $containerFactory = new ContainerFactory(getcwd());
        $this->container = $containerFactory->create(
-            SimpleParameterProvider::provideStringParameter(Option::CONTAINER_CACHE_DIRECTORY),
+            sys_get_temp_dir() . '/phpstan-container-for-rector',


containerCacheDirectory() is used for user that doesn't have access to the sys_get_temp_dir() specific dir, and also for multi user purpose, this cause conflict, that's why it is needed, see issue:

multiple users + [ERROR] Unable to create file ... rector#8112

also see original issue:

WhileEachToForeachRector shows that it ran but it missed the following. How do i fix my config? rector#7848

raised because of it, the default container cache dir is not writable because of no access or internal PHP bug on sys_get_temp_dir() on specific OS/platform.

see original PR that introduce:

[Container] Add RectorConfig::containerCacheDirectory() #3490

it cover usage on shared hosting usage which user can't have access to sys_get_temp_dir() due to shared usage., that's why containerCacheDirectory() options set is needed.

Also, by define this, user will got error:

[ERROR] The file "/var/folders/dv/m3gvpzh94t59l94qz3w2dgfr0000gn/T/phpstan-container-for-rector" does not exist.

The create() doesn't automatically create directory target, it need to be exists before.

I see ;). We can go for defaults here And alkw override by user.

what do you mean? I tried this and got error "phpstan-container-for-rector" directory not exist immediatelly, then if multi user is used, it will crash as can't be read by other user, see issue:

multiple users + [ERROR] Unable to create file ... rector#8112

I've added note about PHPStan container so this is known right in the configuration 👍

Looking at rectorphp/rector#8112, the 2 cache directory options are pretty confusing. There should be just one method to handle both.

See what's @stefanides usage of multi user cause container crash error, using configurable container path resolve based on his experience:

rectorphp/rector#8112 (comment)

for what he experienced.

config/config.php

samsonasik · 2023-08-17T07:44:45Z

@TomasVotruba please don't remove it, it will crash in the first try, as:

Even normal user, user need to manually create /tmp/phpstan-container-for-rector, or it will crash:

 [ERROR] The file "/var/folders/dv/m3gvpzh94t59l94qz3w2dgfr0000gn/T/phpstan-container-for-rector" does not exist.

On shared hosting, when every user has different access, or don't have access to /tmp dir, it will cause error .

samsonasik · 2023-08-17T07:53:08Z

I tried locally, by this change, it doesn't work, PHPStan need to fill cache directory to : /tmp/phpstan-container-for-rector which not yet exists:

 [ERROR] The file "/var/folders/dv/m3gvpzh94t59l94qz3w2dgfr0000gn/T/phpstan-container-for-rector" does not exist.

For user that doesn't have access to that dir, or require multi user purpose, that will blocked to be used, see issue:

multiple users + [ERROR] Unable to create file ... rector#8112 (comment)

…container, only by PHPStan

TomasVotruba · 2023-08-17T08:40:14Z

Even normal user, user need to manually create /tmp/phpstan-container-for-rector, or it will crash:

We should have this test in CI. Could add simple reproducer to check this?

TomasVotruba · 2023-08-17T08:40:43Z

I tried locally, by this change, it doesn't work, PHPStan need to fill cache directory to : /tmp/phpstan-container-for-rector which not yet exists:

The directory should be created then. I'll add it 👍

samsonasik · 2023-08-17T08:53:45Z

@TomasVotruba Merge cache location is causing error:


There were 2 errors:

1) Rector\Tests\NodeTypeResolver\PerNodeTypeResolver\NameTypeResolver\NameTypeResolverTest::test with data set #0
InvalidArgumentException: Failed to create directory "/tmp/rector_cached_files/cache/PHPStan" (unknown cause).

see https://github.com/rectorphp/rector-src/actions/runs/5888776799/job/15970657037#step:5:26

TomasVotruba · 2023-08-17T08:54:33Z

Looking into this. That seems like PHPStan dependency. Give me some time, I'll ask you for more feedback when ready 👍

samsonasik · 2023-08-17T09:07:35Z

packages/Config/RectorConfig.php

+    /**
+     * @info Rector no longer uses compiled container, but Laravel lazy one.
+     * This option is for PHPStan container in case the sys_get_temp_dir() is not available for the current user.
+     */
    public function containerCacheDirectory(string $directoryPath): void


if config cache merged, this method can be deprecated and removed on 1.0 as Option::CONTAINER_CACHE_DIRECTORY no longer be used if cache directory merged.

samsonasik · 2023-08-17T09:14:17Z

Note to myself: after this applied, this need to be tried in local on switch on different projects with and without --clear-cache to test if cache not conflict each other.

samsonasik · 2023-08-17T09:20:09Z

The new "unknown cause" error is equal with issue:

Failed to create directory "/tmp/cache/PHPStan" (unknown cause) rector#8139

TomasVotruba · 2023-09-15T09:37:38Z

Closing for now, as our focus is somewhere else. This should be revisited once AbstractRector is lighter

TomasVotruba changed the title ~~deprecate RectorConfig containerCacheDirectory() as no longer used by container, cleanup phpstan errors~~ [DX] Deprecate RectorConfig containerCacheDirectory() as no longer used by container, cleanup phpstan errors Aug 17, 2023

TomasVotruba changed the title ~~[DX] Deprecate RectorConfig containerCacheDirectory() as no longer used by container, cleanup phpstan errors~~ [DX] Deprecate RectorConfig containerCacheDirectory() as no longer used by container Aug 17, 2023

samsonasik reviewed Aug 17, 2023

View reviewed changes

config/config.php Show resolved Hide resolved

TomasVotruba force-pushed the tv-deprecated-directory branch from 752dc4e to b136b1b Compare August 17, 2023 08:37

Inform RectorConfig containerCacheDirectory() is no longer by Rector …

4637080

…container, only by PHPStan

TomasVotruba force-pushed the tv-deprecated-directory branch from b136b1b to 4637080 Compare August 17, 2023 08:37

TomasVotruba force-pushed the tv-deprecated-directory branch from 81625f9 to 8b3c539 Compare August 17, 2023 08:44

TomasVotruba mentioned this pull request Aug 17, 2023

multiple users + [ERROR] Unable to create file ... rectorphp/rector#8112

Closed

TomasVotruba changed the title ~~[DX] Deprecate RectorConfig containerCacheDirectory() as no longer used by container~~ [DX] Merge RectorConfig cacheDirectory() and containerCacheDirectory() to one to ease changing paths in case of non-writeable cache Aug 17, 2023

make sure PHPStan cache directory is configurable and created

84d4fe5

TomasVotruba force-pushed the tv-deprecated-directory branch from 8b3c539 to 84d4fe5 Compare August 17, 2023 08:49

samsonasik reviewed Aug 17, 2023

View reviewed changes

TomasVotruba closed this Sep 15, 2023

TomasVotruba deleted the tv-deprecated-directory branch September 15, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DX] Merge RectorConfig cacheDirectory() and containerCacheDirectory() to one to ease changing paths in case of non-writeable cache #4803

[DX] Merge RectorConfig cacheDirectory() and containerCacheDirectory() to one to ease changing paths in case of non-writeable cache #4803

TomasVotruba commented Aug 17, 2023 •

edited

samsonasik Aug 17, 2023

samsonasik Aug 17, 2023

samsonasik Aug 17, 2023 •

edited

samsonasik Aug 17, 2023

TomasVotruba Aug 17, 2023

samsonasik Aug 17, 2023

TomasVotruba Aug 17, 2023

TomasVotruba Aug 17, 2023 •

edited

samsonasik Aug 17, 2023

samsonasik commented Aug 17, 2023

samsonasik commented Aug 17, 2023 •

edited

TomasVotruba commented Aug 17, 2023

TomasVotruba commented Aug 17, 2023

samsonasik commented Aug 17, 2023

TomasVotruba commented Aug 17, 2023

samsonasik Aug 17, 2023

samsonasik commented Aug 17, 2023

samsonasik commented Aug 17, 2023

TomasVotruba commented Sep 15, 2023

[DX] Merge RectorConfig cacheDirectory() and containerCacheDirectory() to one to ease changing paths in case of non-writeable cache #4803

[DX] Merge RectorConfig cacheDirectory() and containerCacheDirectory() to one to ease changing paths in case of non-writeable cache #4803

Conversation

TomasVotruba commented Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik commented Aug 17, 2023

samsonasik commented Aug 17, 2023 • edited

TomasVotruba commented Aug 17, 2023

TomasVotruba commented Aug 17, 2023

samsonasik commented Aug 17, 2023

TomasVotruba commented Aug 17, 2023

Choose a reason for hiding this comment

samsonasik commented Aug 17, 2023

samsonasik commented Aug 17, 2023

TomasVotruba commented Sep 15, 2023

TomasVotruba commented Aug 17, 2023 •

edited

samsonasik Aug 17, 2023 •

edited

TomasVotruba Aug 17, 2023 •

edited

samsonasik commented Aug 17, 2023 •

edited