Implemented HttpAdapter with guzzle #178
Conversation
| * @return array | ||
| */ | ||
| public function getMimetype($path) | ||
| { |
There was a problem hiding this comment.
I don't like this implementation: We are actually fetching the metadata from the remote location, but this might already be cached and we don't use the cached version.
Any suggestions on how to improve this?
There was a problem hiding this comment.
This is actually fine, I have to go over the other things first to see if everything is correct though. The caching mechanism makes sure that every bit of metadata returned is cached and it doesn't cache by call but by normalised key. So if you've fetched all the meta-data you won't have to refetch it when just asking for the timestamp or mime-type. The mimetype key will just be fetched from cache.
|
Why not guzzle 4? It's not recommended by the guzzle team to write new code with a deprecated version. Also, you are missing the composer additions. Also, why the extra block comments? They are not consistent with the rest of this repo. |
|
Because I did not notice it was not on Composer, I just used the version that was already in the vendors. I've updated composer.json, composer.json.hhvm, and the code. However the tests are failing in PHP 5.3 because guzzle4 requires PHP 5.4. How do you want me to fix this? Shall I add the php 5.4 requirement to composer.json and update the branch-alias ? The extra block comments are my default setup, I find that it makes the class MUCH more readable. I do not consider consistency to be a relevant point in this case, because consistency should not be a reason to block change when it is considered beneficial, which you could consider, but obviously don't. Anyway, I removed the evil comments as per your request. ;) I noticed that composer.lock is in gitignore. Do you think that is wise? Shouldn't we, by default, install versions of the libraries, tested with Flysystem, instead of the latest versions (which might not have been tested with Flysystem)? |
|
For the php 5.4 stuff, maybe we could swap out the composer.json file like we do for hhvm and skip the guzzle dependant tests? As for the block comments, I think you should remove them, then add them everywhere in a different pull to keep consistency within the code base, but it's not my call. Ping @FrenkyNet. Nah, the composer.lock isn't really needed as far as I can see. I think the idea here is just to fire the tests off against the latest versions to check flysystem is working ok with them. |
|
@GrahamCampbell @hgraca I'm thinking of raising the minimum php version to 5.4 anyway, I just have to determine the right time for that. It seems like other packages and frameworks are making the same move. Laravel, Guzzle, FuelPHP, and CakePHP even are making that move, so might be the right time now. This would eliminate that problem entirely. |
|
@FrenkyNet Should @hgraca just bump the min php version to 5.4 in this pull, and remove 5.3 from travis then? |
|
@hgraca If you can rebase against the latest changes on the master, the php 5.3 issue has been resolved. Basically, it's been removed from travis.yml and php 5.4 has been added as a min version to composer. |
|
Ok, cool! |
|
Sounds great. Thanks. |
|
Updated the PR, you can take a look to see it's ok. |
| <?php | ||
|
|
||
| namespace League\Flysystem\Adapter; | ||
|
|
There was a problem hiding this comment.
There should be one new line here, not 2.
There was a problem hiding this comment.
MAANNN!!! You are very pickyyyy!!! :D But I will change it anyway.
|
What happened to all the docblock parameter types? |
|
@GrahamCampbell I don't understand what you are referring to, can you elaborate, please? |
| * Write a file | ||
| * It's not possible to write to HTTP so it will always do nothing and return false | ||
| * | ||
| * @param $path |
There was a problem hiding this comment.
This is what I mean. We've lost the string.
|
@GrahamCampbell Got it. Updated all docblocks. |
| * | ||
| * @return array|bool | ||
| */ | ||
| public function readStream($path) |
There was a problem hiding this comment.
Why are you not using guzzle in this function? It's possible to get the stream from a request body like so:
$stream = $response->getBody()->getStream();There was a problem hiding this comment.
Btw, if you convert it to use guzzle you can also get the other meta-data from the call which is good for performance in the broader spectrum.
There was a problem hiding this comment.
When I do "$response->getBody()->" I don't get any "getStream()" function.
I did see it in the previous version of guzzle, but not in the latest one.
How would we pass the fetched metadata to the caching system?
There was a problem hiding this comment.
I think we might have to add a method to pipe the response body (which implements the guzzle/streams' StreamInterface to a tmpfile stream. Something along the lines of this:
protected function streamToResource(StreamInterface $stream, $length = 1024)
{
$resource = tmpfile();
while ( ! $stream->oef()) {
fwrite($resource, $stream->read($length));
}
return $resource;
}Also, when the adapters the same thing as the read method does, returning an array with all the data including the stream, the Filesystem class will make sure the caching receives that.
There was a problem hiding this comment.
But then, won't we be downloading the whole file?
Doesn't that defeat part of the purpose of having a stream?
Also, as I understand, the caching (in the Filesystem class) will use the stream to get the metadata. So if we provide a stream to the tmp file, wont the metadata be wrong?
There must be a more elegant way of doing this... I will ask the guzzle project about it.
There was a problem hiding this comment.
For when you've got more time to deal with this, this is what we'll need to use in order to convert the StreamInterface to a resource: https://github.com/guzzle/streams/blob/master/src/GuzzleStreamWrapper.php#L27
There was a problem hiding this comment.
That's what I did in my last pull request :)
I'm starting to review the comments on that PR now.
|
I finally found a nice way of doing this and updated the pull request. |
| * | ||
| * @return self | ||
| */ | ||
| public function setClient(Client $client) |
There was a problem hiding this comment.
why is it important to set the Client later on...? I think it should be required while creation/instantiation -> Immutability
There was a problem hiding this comment.
It would be better to typehint it as ClientInterface anyway.
There was a problem hiding this comment.
@staabm I think that the fact that we cant think of a reason now, doesn't mean there won't be a reason to do it at some point. I agree that it is indeed unlikely, but nevertheless... Also, I'm using it inside the constructor, and I don't like accessing properties directly, even if inside the own class.
@GrahamCampbell You are totally right, I will change it.
|
Should we really be typehinting the EDIT: typewriting => typehinting |
|
since |
|
Its a good practice to depend on abstractions whenever possible... |
|
@FrenkyNet |
|
This needs rebasing. |
| $timestamp = $response->getHeader('Last-Modified'); | ||
| $contentType = $response->getHeader('Content-Type'); | ||
|
|
||
| $size = "unknown"; |
There was a problem hiding this comment.
This default value should be removed, it won't be added to the return value if you use compact instead of making the array yourself.
|
@hgraca it'll need a rebase before I can merge. I think the rest looks good. We can improve stuff over time to. Like supports for writes. |
|
I'm assuming this new feature will make the next release 0.6.0. |
|
@GrahamCampbell because it's still a 0.x release cycle this won't be needed. It won't break any integrations. As soon as we hit 1.x, which I want to work to in the near future. |
|
Even during 0.x you're meant to bump the minor version if you add features. aren't you? The only difference with 0.x is that bumping the minor version also is allowed to introduce bc breaks. |
|
@GrahamCampbell the versioning all moves up a dot, which would make this not a minor, but a patch release. I've maintained pretty strict versioning for Flysystem as a 0.x package, wouldn't you agree? ;) |
|
Semantic versioning says:
What I get from that is that 0.6.0 is needed now? |
|
What I get from that is that there are no actual rules, with an added adapter the public API doesn't change. |
|
Yeh, it does say |
|
@GrahamCampbell agreed, will do that. |
|
Great, i will try to do that during the weekend. I haven't had the time to follow upon this, was on vacations and i am now on a job quest. Enviado de Samsung Mobile -------- Mensagem original -------- @hgraca it'll need a rebase before I can merge. I think the rest looks good. We can improve stuff over time to. Like supports for writes. — |
|
If you like, I can rebase this and send another pull, retaining your name on the commit? |
|
If you have the time for it, please do. Thank you. Herberto Graça http://pt.linkedin.com/pub/herberto-gra%C3%A7a/8/10/b32 2014-09-02 15:21 GMT+02:00 Graham Campbell notifications@github.com:
|
|
I'll have a look at it asap. |
|
@hgraca have you changed your github email address? Github is no longer reconsigning you on any commits. |
|
|
|
Hi, That is my account with my current employer, I hadn't set it up with my Herberto Graça http://pt.linkedin.com/pub/herberto-gra%C3%A7a/8/10/b32 2014-09-02 22:55 GMT+02:00 Graham Campbell notifications@github.com:
|
|
Merged this into a feature branch. |
|
Cool! Herberto Graça http://pt.linkedin.com/pub/herberto-gra%C3%A7a/8/10/b32 2014-09-06 12:03 GMT+02:00 Frank de Jonge notifications@github.com:
|
|
@hgraca something went weirdly wrong in merging it into a feature branch, I've pulled it out and put it in a feature branch manually, sorry for the inconvenience. |
I'm gonna add some comments to the code, would like for you guys to let me know what you think about it.