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

Add a MultiInstanceFile to allow many file handles to the same resource #220

Merged
merged 2 commits into from Feb 24, 2020

Conversation

@bizurkur
Copy link
Member

bizurkur commented Feb 20, 2020

This should allow any number of handles to be opened to the same vfs resource, but each maintain separate pointers to the place in the file they are.

  • Handle A should know it's at position X
  • Handle B should know it's at position Y
  • Each file handle should not interfere with the other file handle's position when performing operations

In other words, this is now possible (how it works with normal files):

$root = vfsStream::setup();
$file = vfsStream::newFile('foo')->setContent('baz2')->at($root);

$fp1 = fopen($file, 'rb');
$fp2 = fopen($file, 'rb');

$this->assertEquals('baz2', fread($fp1, 4096));

// previously this would be a blank string, as fp1 read all the data
$this->assertEquals('baz2', fread($fp2, 4096)); 

fclose($fp1);
fclose($fp2);

Closes #129

@bizurkur bizurkur requested a review from bovigo/vfsstream Feb 20, 2020
Copy link
Member

mikey179 left a comment

Overall I'm not against this, as this solves a bug in the library and doesn't introduce anything that clutters the public API. My only gripe with it right now is the name MultiInstanceFile.

Having said that, for me it's another piece in the puzzle of how things could be restructured internally. I can't put them all together in my mind quite yet, but my idea for improving the class names in #213 is part of that. Especially the implementation of restorePosition() and savePosition() show that there's massive room for improvement and that maybe not all informations are at the place where they ought to be.

src/MultiInstanceFile.php Outdated Show resolved Hide resolved
@bizurkur bizurkur requested a review from mikey179 Feb 22, 2020
@bizurkur bizurkur merged commit 7979e9d into bovigo:master Feb 24, 2020
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 94.782%
Details
@bizurkur bizurkur deleted the bizurkur:issue-129 branch Feb 24, 2020
@mikey179 mikey179 mentioned this pull request Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.