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

initial 'smb' streams support #20

Merged
merged 14 commits into from Sep 8, 2015

Conversation

@remicollet
Copy link
Contributor

remicollet commented Sep 5, 2015

Please don't merge this, initial work in progress
Open for discussion.

Most changes in sources are factorization (to avoid copy/paste in streams wrapper)

@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Sep 6, 2015

Start to be usable, any comment on this feature ?

@aklomp

This comment has been minimized.

Copy link
Collaborator

aklomp commented Sep 6, 2015

Excellent work! Looks solid to me. If I had more experience with PHP internals I could give better feedback. Here are some first thoughts:

  • The new code uses php_error_docref(), not php_error(), for printing errors. Is one superior to the other, and should we switch in the whole codebase?
  • The stream feature needs documentation in the README.
  • When you're satisfied, please squash your commits so that history is linear (no changes by previous commits are overwritten by later commits).
@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Sep 6, 2015

The new code uses php_error_docref(), not php_error(), for printing errors. Is one superior to the other, and should we switch in the whole codebase?

Mostly eq., IIRC excepted in TS mode where a TSRMLS_FETCH is saved.

@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Sep 6, 2015

When you're satisfied, please squash your commits so that history is linear (no changes by previous commits are overwritten by later commits).

I don't know how to do this simply, except by closing this PR and open a new one with a single commit... :(

@aklomp

This comment has been minimized.

Copy link
Collaborator

aklomp commented Sep 6, 2015

Just force-push over your issue-stream branch. GitHub will update the pull request automatically.

By "squash" I don't mean merging everything into a single commit, but creating a couple of linear-looking commits that together implement this feature. It's just a preference, we could also merge the work as it is now.

@remicollet remicollet force-pushed the remicollet:issue-stream branch from f55cf99 to 637118c Sep 6, 2015
@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Sep 6, 2015

I think all possible features are now implemented (latest, chmod, touch, are 5.4+ only).

@remicollet remicollet force-pushed the remicollet:issue-stream branch from 4252b1e to 7e7f0c4 Sep 6, 2015
@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Sep 7, 2015

On my side, everything is done.

Ready for review / merge.

Notice, I have change version to 0.8.0-dev.
If merged, probably worth to set to 0.8.0beta and tag a pre-release version, to get user feedback.

@aklomp

This comment has been minimized.

Copy link
Collaborator

aklomp commented Sep 7, 2015

Hi Remi, thanks for the excellent, high quality work. You even copied the project's coding style, nice. I think this can be merged without further discussion. At the most I think the README could use a code example of this new functionality. But we can always add one later. Eduardo?

@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Sep 7, 2015

At the most I think the README could use a code example of this new functionality.

Isn't 01e2401 enough ?
But yes other can be added later.

Perhaps, could recommend to use the functions instead streams for massive action (single context, instead of new context for each command)

@aklomp

This comment has been minimized.

Copy link
Collaborator

aklomp commented Sep 7, 2015

With the examples thing I meant put full code samples in the Examples section. But maybe the current example is enough for demonstration, since the streams API is more for one-shot use anyway. We can always add examples later.

Your contribution looks excellent, but I have a few questions/comments, which I will add as annotations.

@aklomp

This comment has been minimized.

Copy link

aklomp commented on README.md in 01e2401 Sep 7, 2015

This could be a level 2 heading (two #'s) to separate it from the API section. (Apologies for the low quality of my criticism :)

@aklomp

This comment has been minimized.

Copy link

aklomp commented on smb_streams.c in 20f2d81 Sep 7, 2015

Is there a reason why this function does not have the guard statement:

if (!self || !self->handle) {
    return 0;
}

(Same for php_smb_ops_seek)

This comment has been minimized.

Copy link
Owner Author

remicollet replied Sep 8, 2015

Well, plainwrapper.c even use assert(self != NULL)
Guard statement added where missing.

@aklomp

This comment has been minimized.

Copy link

aklomp commented on smb_streams.c in 20f2d81 Sep 7, 2015

About the return values: are 0 and -1 the canonical return values? In for instance php_stream_smb_mkdir and php_stream_smb_rmdir they are 0 and 1.

This comment has been minimized.

Copy link
Owner Author

remicollet replied Sep 8, 2015

Yes, I hink, they are the canonical values (most skeleton copied from php plain wrapper sources)
And yes, they sometime look inconsistents ;)

@aklomp

This comment has been minimized.

Copy link

aklomp commented on smb_streams.c in 7e7f0c4 Sep 7, 2015

The indent level is getting high. Maybe set ret to -1 at the start of the function and only set it to 0 if the operation succeeds? Then you can do:

int ret = -1;

...

if ((smbc_open = smbc_getFunctionOpen(state->ctx)) == NULL) {
    break;
}
if ((smbc_close = smbc_getFunctionClose(state->ctx)) == NULL) {
    break;
}
if ((smbc_utimes = smbc_getFunctionUtimes(state->ctx)) == NULL) {
    break;
}
if ((handle = smbc_open(state->ctx, url, O_EXCL|O_CREAT, 0666)) != NULL) {
    smbc_close(state->ctx, handle);
    ret = 0;
}
if (newtime) {
    ...
    ret = smbc_utimes(state->ctx, url, times);
}
break;
@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Sep 8, 2015

The "cleanups" commit should take care of all comments.

aklomp added a commit that referenced this pull request Sep 8, 2015
Support for `smb://` streams in PHP
@aklomp aklomp merged commit f957858 into eduardok:master Sep 8, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aklomp

This comment has been minimized.

Copy link
Collaborator

aklomp commented Sep 8, 2015

Merged, and thank you for the great contribution!

@remicollet remicollet deleted the remicollet:issue-stream branch Sep 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.