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

Make drivers more consistent / reduce driver API #38

Merged
merged 33 commits into from
Jun 23, 2020

Conversation

enumag
Copy link
Contributor

@enumag enumag commented Mar 6, 2020

No description provided.

src/BlockingDriver.php Outdated Show resolved Hide resolved
@enumag enumag force-pushed the feature/improve-doc-blocks branch from bb3ad96 to 6b30218 Compare April 26, 2020 13:44
test/DriverTest.php Outdated Show resolved Hide resolved
@enumag
Copy link
Contributor Author

enumag commented May 1, 2020

ping @kelunik

src/Driver.php Outdated Show resolved Hide resolved
src/functions.php Outdated Show resolved Hide resolved
test/DriverTest.php Outdated Show resolved Hide resolved
@enumag
Copy link
Contributor Author

enumag commented May 2, 2020

@kelunik Most likely chown needs the same treatment with if ($this->priorVersion) { as most of the other methods. This might apply to chmod and possibly other methods that are not covered by tests as well.

@kelunik
Copy link
Member

kelunik commented May 2, 2020

@enumag The test won't work that way due to umask.

@enumag
Copy link
Contributor Author

enumag commented May 2, 2020

@kelunik Is there a way to fix it or do I have to remove it?

@kelunik
Copy link
Member

kelunik commented May 2, 2020

@enumag I guess $old = umask(0) + umask($old) is needed, as it's used here: https://www.php.net/manual/en/function.umask.php

@enumag enumag force-pushed the feature/improve-doc-blocks branch from 790d526 to ed5ccf3 Compare May 2, 2020 10:13
@enumag
Copy link
Contributor Author

enumag commented May 2, 2020

@kelunik Actually umask wasn't the problem. Missing clearstatcache was. Second call to fileperms returned wrong result.

test/DriverTest.php Outdated Show resolved Hide resolved
@enumag enumag force-pushed the feature/improve-doc-blocks branch from 1a0bcf4 to 1a7f4c0 Compare May 2, 2020 10:29
@enumag enumag changed the title Improve doc blocks Improve doc blocks + new tests + fixed UvDriver issues May 2, 2020
@enumag enumag changed the title Improve doc blocks + new tests + fixed UvDriver issues Improve doc blocks + fixed UvDriver May 2, 2020
@enumag enumag force-pushed the feature/improve-doc-blocks branch from 1310ea4 to 9fdedb6 Compare May 2, 2020 10:45
@enumag enumag marked this pull request as draft May 2, 2020 10:45
Co-authored-by: Niklas Keller <me@kelunik.com>
src/UvDriver.php Outdated Show resolved Hide resolved
@enumag enumag force-pushed the feature/improve-doc-blocks branch from 86b2aab to b0105ce Compare May 19, 2020 09:44
enumag and others added 4 commits May 19, 2020 11:45
Co-authored-by: Niklas Keller <me@kelunik.com>
Co-authored-by: Niklas Keller <me@kelunik.com>
@enumag enumag requested a review from kelunik May 19, 2020 10:02
@enumag
Copy link
Contributor Author

enumag commented May 21, 2020

@bwoebi Could you take a look what's wrong with UvDriver::link() please? It still hangs the tests and I can't figure it out. I actually kinda suspect there might be something wrong in ext-uv.

@kelunik kelunik requested a review from trowski June 23, 2020 04:28
src/UvDriver.php Outdated Show resolved Hide resolved
test/Fixture.php Outdated Show resolved Hide resolved
test/DriverTest.php Outdated Show resolved Hide resolved
test/DriverTest.php Outdated Show resolved Hide resolved
test/DriverTest.php Outdated Show resolved Hide resolved
@kelunik
Copy link
Member

kelunik commented Jun 23, 2020

@enumag I've fixed UvDriver::link, it had a parameter too much.

@trowski Please review, I'd like to finally merge this.

@kelunik kelunik changed the title Consistent drivers Make drivers more consistent / reduce driver API Jun 23, 2020
@kelunik kelunik merged commit 0ed53c6 into amphp:master Jun 23, 2020
@enumag
Copy link
Contributor Author

enumag commented May 24, 2021

@kelunik @trowski Could we have a release with these changes?

@kelunik
Copy link
Member

kelunik commented Jun 4, 2021

@enumag Yes, we should indeed finally tag v2. I guess the code is ready, we just need to write a changelog. I originally wanted to wrap stat into an object, but probably won't do that. Do you want to help with the changelog?

@enumag
Copy link
Contributor Author

enumag commented Jun 4, 2021

@kelunik Well the notable changes are these:

- Added AsyncFileMutex (#43)
- Consistent and reduced drivers API (#38)
- Redesigned API (#53)

Then it depends on if you want to

@kelunik
Copy link
Member

kelunik commented Jun 4, 2021

That's a good starting point, yes. I think listing the renames makes senses, as it only needs to be done once, otherwise everybody will have to spend time looking things up. I'm not sure whether detailed changelogs for the driver interface makes sense, as there probably aren't many people who implemented their own driver. Do you want to work on that?

@enumag
Copy link
Contributor Author

enumag commented Jun 4, 2021

@kelunik here is what I gathered:

Renamed:
open -> openFile
stat -> getStatus
lstat -> getLinkStatus
size -> getSize
isdir -> isDirectory
isfile -> isFile
mtime -> getModificationTime
atime -> getAccessTime
ctime -> getCreationTime
symlink -> createSymlink
link -> createHardlink
readlink -> resolveSymlink
rename -> move
unlink -> deleteFile
rmdir -> deleteDirectory
scandir -> listFiles
chmod -> changePermissions
get -> read
put -> write

Added:
isSymlink

Changed:
mkdir split to createDirectory and createDirectoryRecursively
chown renamed to changeOwner, $uid and $gid now accept null instead of -1 to ignore

@enumag
Copy link
Contributor Author

enumag commented Jun 12, 2021

ping @kelunik Anything else I can help with here?

@kelunik
Copy link
Member

kelunik commented Jun 12, 2021

No, thank you! I'll get it tagged in the coming days. Sorry about the delay.

@kelunik
Copy link
Member

kelunik commented Jun 21, 2021

I finally got to my notebook and tagged the release, thank you so much!

@enumag
Copy link
Contributor Author

enumag commented Jun 22, 2021

Thanks @kelunik!

I noticed a few things when browsing the latest commits.

  1. You're apparently using a dev version of ext-uv (f45eb11). How are things looking there? Could we have a new release?

  2. Also noticed that you're ignoring ext-eio on PHP 8 (90f9550). However iirc there is already a PHP 8 compatible RC release of ext-eio.

I'll try to find some time to update amphp/cache and amphp/http-client.

@kelunik
Copy link
Member

kelunik commented Jun 23, 2021

I'm using the current master branch of ext-uv, yes. I'm not sure what changes from the last release there.

I'm ignoring eio, because the setup-php script doesn't install it currently. If you have a fix for that, please send a PR.

@enumag enumag mentioned this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants