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

pull request for issue #65 #73

Open
wants to merge 2 commits into
base: 2.4.x
Choose a base branch
from
Open

pull request for issue #65 #73

wants to merge 2 commits into from

Conversation

tck
Copy link

@tck tck commented Sep 5, 2023

No description provided.

Signed-off-by: tck <toby3@gmx.de>
Signed-off-by: tck <toby3@gmx.de>
Comment on lines +117 to +119
if (null === $this->cacheDir) {
$this->setCacheDir(null);
}
Copy link
Member

Choose a reason for hiding this comment

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

Needs some sort of test / reproducer, IMO.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest creating a directory in that test, change permissions to read-only using chmod and pass that to ini_set to modify sys temp dir for that test.

Having something like try {} finally {} to restore temp dir back to default to ensure test does not introduce side-effects to other tests.

Copy link

@bcremer bcremer Jan 26, 2024

Choose a reason for hiding this comment

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

sys_temp_dir is of type PHP_INI_SYSTEM and thus cannot be set during runtime via ini_set.

Setting the TMPDIRenvironment variable works:
putenv('TMPDIR=/fasel').

Edit:
Seems like putenv('TMPDIR=/fasel') will also not work in the phpunit context.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if this is a tmpdir only issue. Whenever a directory is used without write-permissions, that could lead to problems, no?

@boesing boesing linked an issue Sep 6, 2023 that may be closed by this pull request
@boesing boesing added Bug Something isn't working Unit Test Needed labels Sep 6, 2023
@sbenischke
Copy link

Any update on this topic?

In my opinion the sys_tmp_dir should be used as a fallback after the try to initialize the configured directory. So at least there is the possibility to get this adapter working, if the sys_tmp_dir is neither readable or writeable.

@boesing
Copy link
Member

boesing commented Nov 29, 2023

Once the requested changes are applied, we can move on.
Feel free to use these changes to create a new PR with added test(s) or wait until the author is responding.

@bcremer
Copy link

bcremer commented Jan 26, 2024

I stumbled about this problem during a docker run --read-only deployment of an mezzio application.

I came up with the same fix as @tck. Building a reproducer as phpunit testcase is tricky because sys_tmp_dir cannot be modified during runtime, see: #73 (comment).

A standalone reproducer looks like the following:

<?php

require __DIR__  . '/vendor/autoload.php';

putenv('TMPDIR=/fasel');

// the following will throw an exception bacause `/fasel` does not exists even though the cache dir was explicitly overwritten
new \Laminas\Cache\Storage\Adapter\FilesystemOptions(['cacheDir' => '/tmp']);
PHP Fatal error:  Uncaught Laminas\Cache\Exception\InvalidArgumentException: Cache directory '/fasel' not found or not a directory in /home/apidev/current/laminas-cache-storage-adapter-filesystem/src/FilesystemOptions.php:440

If nobodoy has an idea how to codify the reproducer in phpunit I would suggest to move on and merge the PR as is.

@boesing
Copy link
Member

boesing commented Jan 26, 2024

Might be possible to modify the tmpdir setting via php CLI command when running a specific test group.

We could create a test, add a group to it, use that group in additional_checks when running php vendor/bin/phpunit along with the -d parameter to modify the temp dir target, add a check to that test that it skips the execution when tmp directory is readable and that should do the trick, no?

The check in the test could look like:

if (is_writable(sys_get_temp_dir())) { 
    self::markAsRisky(); 
    self::markTestSkipped('Test has to be executed in read-only environment...');
}

Here is an example on how addtional_checks could look like:

https://github.com/laminas/laminas-cache/blob/ce069bc6c0c10abf13a51da1b0153090dc1caf0c/.laminas-ci.json#L4-L11

@bcremer
Copy link

bcremer commented Jan 29, 2024

@boesing Implemented in a separate PR: #84

@tck tck requested a review from Ocramius February 11, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Unit Test Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache type not usable if global sys_tmp_dir not writeable
5 participants