Stream filter seeking #19981
Stream filter seeking #19981
Conversation
|
Just to note that the current implementation is just an API draft that is not really tested. It's also does not handle the buffered procession and no current filter implements it. Also it will need proper tests. |
bukka
force-pushed
the
stream_filter_seeking
branch
from
e78adee to
41aee5a
Compare
|
This is now ready for review. |
|
This is just a heads up that I plan to merge it tomorrow unless someone wants more time for review or finds anything that needs to be addressed. Of course, I'm happy to address any potentially issue even if found after merging. I just don't want to wait for review forever as this needs to get merged (part of my stream work)... |
| RETURN_LONG(PSFS_ERR_FATAL); | ||
| } | ||
|
|
||
| PHP_METHOD(php_user_filter, seek) |
There was a problem hiding this comment.
What does this mean for the check you added earlier that checks if "seek" is part of the function_table?
There was a problem hiding this comment.
So this is because the filter is not required to be subclass of php_user_filter so if it isn't its subclass and it doesn't implement it, we return true for BC. If it is subclass of php_user_filter, then I thought it might be good to have it implemented so the child class can call parent::seek.
There was a problem hiding this comment.
Okay I see. In the future I feel like we should require subclassing php_user_filter.
There was a problem hiding this comment.
Yeah might be good candidate for deprecation and PHP 9 change...
bukka
force-pushed
the
stream_filter_seeking
branch
from
73bcbd9 to
f6f6152
Compare
|
@ndossche Thanks a lot for the review. Hopefully all comments addressed. |
| } | ||
|
|
||
| /* Make sure the stream is not closed while the filter callback executes. */ | ||
| uint32_t orig_no_fclose = stream->flags & PHP_STREAM_FLAG_NO_FCLOSE; |
There was a problem hiding this comment.
Perhaps I miss something here, but can't you move this flag update right before "Setup calling arguments" ?
Then you can clean up the error handling in the "userfilter_assign_stream" failure path as well.
There was a problem hiding this comment.
And same for the other caller of userfilter_assign_stream
There was a problem hiding this comment.
I thought that it might cleaner not to pass orig_no_fclose but doesn't really matter. So changed as suggested.
There was a problem hiding this comment.
No, I perhaps didn't express myself good enough.
What I meant was to move lines 283-284 to after userfilter_assign_stream, but now I see that idea is actually bogus because of property hooks. So perhaps the original code was better although the current code also seems fine.
2 participants
Currently filter seeking does not work correctly for most streams. The idea is to extend API to allow seeking for some streams. There are couple of cases:
string.rot13,string.toupper,string.tolowerThe seeking to zero is implemented by resetting the internal state.
Fixes https://bugs.php.net/bug.php?id=49874