Skip to content

Add uploadFromUrl and restructure other upload methods (fixes #70)#71

Merged
Xymph merged 4 commits intohamstar:masterfrom
Xymph:70-upload-from-url
Nov 27, 2016
Merged

Add uploadFromUrl and restructure other upload methods (fixes #70)#71
Xymph merged 4 commits intohamstar:masterfrom
Xymph:70-upload-from-url

Conversation

@Xymph
Copy link
Copy Markdown
Collaborator

@Xymph Xymph commented Nov 21, 2016

See #70. Previously, upload() took $data and uploadFile() invoked upload(). However, uploadFromUrl() takes $url and otherwise needs exactly the same code as upload(). To allow sharing this common code, I had to restructure upload() a bit into a private method, uploadCommon() (maybe there's a better name for that?), and then its public use is made clearer by a new name, uploadData().

The previous merge is just a few days old (so hasn't spread far yet) and still falls under "current development", so this API change seems acceptable to me, as it leads to consistent naming of the three public upload methods. For further consistency, I renamed download() to downloadData() as well.

@Xymph Xymph force-pushed the 70-upload-from-url branch from 18194b3 to e3cea2b Compare November 21, 2016 13:08
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Nov 21, 2016

Minor code change in addition to the restructuring of 4899d33: is_null($text) is a better way to test input parameters than === null.

Also note that upload from URL goes through the same multipart POST in Wikimate, even though this isn't needed when the request contains no binary data -- but it doesn't hurt to do it that way either.

@Xymph Xymph mentioned this pull request Nov 23, 2016
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Nov 26, 2016

@waldyrious, would you be able to squeeze in a review one of these days? 😉

Comment thread Wikimate.php Outdated
{
if ($refresh) { // We want to query the API

// Specify all image properties to retrieve
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't either this line use "relevant ... properties", or line 529 use "all ... properties"? For consistency.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Well, it gets far more properties than for a page, and I meant it more in the sense of "list all those that are to be retrieved, and don't list those that won't be retrieved". :) But 'relevant' is fine too.

Either way, 'image' should be 'file', so thanks for highlighting this anyway.

Comment thread Wikimate.php
@waldyrious
Copy link
Copy Markdown
Collaborator

Apart from the minor comments I've left inline, this LGTM :)

@waldyrious
Copy link
Copy Markdown
Collaborator

waldyrious commented Nov 26, 2016

@waldyrious, would you be able to squeeze in a review one of these days? 😉

Oh, crap, you were faster :P I had this page open for the last few days and didn't refresh it before starting the review, so I hadn't seen this comment when I started. I'm saying this just to reassure you that I didn't review the PR now following your prompt, but indeed hadn't forgotten :)

@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Nov 27, 2016

Sorry. 😉 I guess all the quick review turnarounds in the past spoiled me a bit, but I thought I'd poll you after 5 days.
Thanks again, time to wrap up this one.

@Xymph Xymph merged commit 70f2c8a into hamstar:master Nov 27, 2016
@Xymph Xymph deleted the 70-upload-from-url branch November 27, 2016 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants