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 ftp_append to create a new file or append data to an existing file (RFC959) #2615

Closed
wants to merge 4 commits into from

Conversation

@blar
Copy link
Contributor

@blar blar commented Jul 8, 2017

Add ftp_append to create a new file or append data to an existing file (RFC959)

@blar
Copy link
Contributor Author

@blar blar commented Jul 8, 2017

The signature:
bool ftp_append ( resource $ftp_stream , string $remote_file , string $local_file , int $mode)

The signature is identical to ftp_put except for the optional argument $startpos.

@KalleZ
Copy link
Member

@KalleZ KalleZ commented Jul 8, 2017

LGTM

@sgolemon thoughts? It's a fairy self contained small feature

@blar
Copy link
Contributor Author

@blar blar commented Jul 8, 2017

All tests pass except for travis-ci "ENABLE_MAINTAINER_ZTS=0 ENABLE_DEBUG=0". I don't see this is related to my changes.

@nikic
Copy link
Member

@nikic nikic commented Jul 18, 2017

Another ping for @sgolemon. Can this go into 7.2?

@nikic
Copy link
Member

@nikic nikic commented Aug 2, 2017

Note for merger: When merging into master the mode needs to be adjusted to be optional and default to binary.

@tpunt
Copy link
Contributor

@tpunt tpunt commented Aug 2, 2017

@nikic Why should the mode be made optional on this function when it is required on all the others? It looks like the arginfo should be changed here to the require 4 args instead of 3, unless I'm missing something?

@blar
Copy link
Contributor Author

@blar blar commented Aug 2, 2017

@tpunt I fix this

@tpunt
Copy link
Contributor

@tpunt tpunt commented Aug 2, 2017

@blar Oh right, now I see...

@sgolemon
Copy link
Contributor

@sgolemon sgolemon commented Aug 2, 2017

Remove the unrelated changes to all the other arginfo structs, and I'll be fine with it.

I'm not saying we can't also fix the arginfos, but that should be a separate diff.

@sgolemon
Copy link
Contributor

@sgolemon sgolemon commented Aug 2, 2017

Specifically, the arginfo values are wrong, and it's a bug. So those fixes should be done on the earliest 7.x branch which contains incorrect values and merged forward from there (which is another good reason to have it in a separate diff)

@sgolemon
Copy link
Contributor

@sgolemon sgolemon commented Aug 2, 2017

Okay, correction. The aginfos on PHP-7.2 do match their function implementations, so this diff, as it stands, is wrong. The arginfos shouldn't be updated without also updating the implementations. At that point the change becomes much less self-contained.

At this point, I would say: Make the new function require the mode argument for consistency with the other ftp functions. Remove the changes to their arginfos, and plan to make mode optional in 7.3

@nikic
Copy link
Member

@nikic nikic commented Aug 2, 2017

@sgolemon The implementation of this PR as it stands now is for master (where the mode arguments are already optional). For PHP 7.2 this would be merged without the last commit and that commit would be applied for master only.

@nikic
Copy link
Member

@nikic nikic commented Aug 2, 2017

Okay, 29e4d4e (the ftp_append addition) is now merged into 7.2. 27c225a (making mode on ftp_append optional) and 60dc329 (fixing arginfo structures) are in master.

@nikic nikic closed this Aug 2, 2017
@sgolemon
Copy link
Contributor

@sgolemon sgolemon commented Aug 2, 2017

Ah, that would explain why the arginfo stucts looked wrong on the website, but not in my checkout of 7.2 :D

Thanks, @nikic

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.

None yet

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