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

Added clear cache for windows, fix tests #8366

Open
wants to merge 1 commit into
base: 1.9
from

Conversation

@proggga
Copy link

commented Oct 8, 2019

No description provided.

@curry684

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

If memory serves me right (wrote this 3 years ago...) this change is irrelevant as is_dir and is_link will not 'lie' as their state is tightly coupled with PHP's native filesystem functions which properly update the caches. The clearstatcache is so important for the lstat because it has to detect the junctions which are created by invoking the external CLI commands, thus bypassing the cache and potentially making it stale.

So, I think the change is irrelevant. Is this fixing an actual issue you are having?

/cc @Seldaek

@proggga

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

Im trying to fix bug in appveiour, looks like maybe it can help

@proggga

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

From php doc: is_dir . and is_link can be cached
https://www.php.net/manual/en/function.clearstatcache.php

Affected functions include stat(), lstat(), file_exists(), is_writable(), is_readable(), is_executable(), is_file(), is_dir(), is_link(), filectime(), fileatime(), filemtime(), fileinode(), filegroup(), fileowner(), filesize(), filetype(), and fileperms().

@curry684

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

Yes but they don't check the bits that can 'lie', the mask we apply on the lstat result can.

@alcohol

This comment has been minimized.

Copy link
Member

commented Oct 17, 2019

Considering there is no drawback to moving it before the methods of which it could possibly affect the outcome, I don't see why this change is irrelevant? Worst case scenario, nothing gained, nothing lost. The bug solving potential is enough IMHO.

@alcohol

This comment has been minimized.

Copy link
Member

commented Oct 17, 2019

@proggga bugfixes should target the latest release branch though (1.9 in this case). Can you adjust and rebase?

@proggga proggga force-pushed the proggga:progga/fix_filesystem_cache_clean branch from e48ff9a to c3cc16e Oct 17, 2019
@proggga proggga force-pushed the proggga:progga/fix_filesystem_cache_clean branch from c3cc16e to ab0cb7c Oct 17, 2019
@proggga proggga changed the base branch from master to 1.9 Oct 17, 2019
@proggga

This comment has been minimized.

Copy link
Author

commented Oct 17, 2019

Adjusted and rebased

@curry684

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2019

I'm fine with the change, I was just explaining that the bug solving potential should be close to zero if PHP is working as documented. That in itself is always a gamble so indeed fine to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.