Refactor SplFileObject methods #6014
Conversation
Girgias
force-pushed
the
spl-dir-refactor
branch
from
2e7dcbf to
5d7a1d7
Compare
ext/standard/file.c
Outdated
| @@ -344,7 +344,9 @@ PHP_FUNCTION(flock) | |||
|
|
|||
| PHP_STREAM_TO_ZVAL(stream, res); | |||
|
|
|||
| act = operation & 3; | |||
| act = operation & PHP_LOCK_UN; | |||
| // TODO doesn't this fail if operation is a bitmask with LOCK_NB? | |||
There was a problem hiding this comment.
Assuming that LOCK_NB==4 and PHP_LOCK_UN==3, the former would have already been masked out.
There was a problem hiding this comment.
I'm not very good at understanding masks so I assume that you're correct
ext/spl/spl_directory.c
Outdated
| @@ -57,6 +52,13 @@ PHPAPI zend_class_entry *spl_ce_GlobIterator; | |||
| PHPAPI zend_class_entry *spl_ce_SplFileObject; | |||
| PHPAPI zend_class_entry *spl_ce_SplTempFileObject; | |||
|
|
|||
| // TODO Use standard Error | |||
| #define CHECK_SPL_FILE_OBJECT_IS_INITIALIZED() \ | |||
There was a problem hiding this comment.
Maybe accept the spl_filesystem_object * as parameter?
ext/spl/config.m4
Outdated
| @@ -1,3 +1,3 @@ | |||
| PHP_NEW_EXTENSION(spl, php_spl.c spl_functions.c spl_engine.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c, no,, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1) | |||
| PHP_INSTALL_HEADERS([ext/spl], [php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h]) | |||
| PHP_ADD_EXTENSION_DEP(spl, pcre, true) | |||
| PHP_ADD_EXTENSION_DEP(standard, spl, pcre, true) | |||
There was a problem hiding this comment.
Are the first two arguments swapped?
|
|
||
| /* {{{ Stat() on a filehandle */ | ||
| PHP_FUNCTION(fstat) | ||
| PHPAPI void php_fstat(php_stream *stream, zval *return_value) |
There was a problem hiding this comment.
Should I make this return a zval* instead of the out param?
There was a problem hiding this comment.
It would be signalled by a IS_FALSE zval* as it's currently the value returned when fstats() fails. Not sure if that's a good idea but signalling it via SUCCESS/FAILURE or true/false seems rather pointless because I would expect anyone using this API to want the result of the fstats call if it succeeded.
There was a problem hiding this comment.
Ah, thanks! If I understand correctly, return_value is an in/out parameter, and that would be fine.
Girgias
force-pushed
the
spl-dir-refactor
branch
from
5d7a1d7 to
49adee9
Compare
Girgias
force-pushed
the
spl-dir-refactor
branch
from
49adee9 to
076414b
Compare
| static int flock_values[] = { LOCK_SH, LOCK_EX, LOCK_UN }; | ||
|
|
||
| /* {{{ Portable file locking, copy pasted from ext/standard/file.c flock() function. | ||
| * This is done to prevent this to fail if flock is disabled via disable_functions */ |
There was a problem hiding this comment.
Is that a bug or a feature? If someone disabled flock(), I imagine they also wouldn't want SplFileInfo::flock() to work, no?
There was a problem hiding this comment.
That's a good question, but most SPL methods are not disabled when the corresponding functional version is disabled. That's why I did this, moreover this was considered as an internal "bug" so if we want to revert this I think any function which is disabled should then disable it's SPL counterpart IMHO
There was a problem hiding this comment.
Either way, copy and paste doesn't look like a good idea to me.
There was a problem hiding this comment.
That's a good question, but most SPL methods are not disabled when the corresponding functional version is disabled.
Good point. If it's the same with other functions, then I'm fine with it.
This is an attempted at making this class a bit easier to follow...
However since PHP 8 disabled functions are unregistered it's possible to hit some new unconventional paths which I tried to drop
however I ran into some issues with[The compatibility layer wasn't completely useful and just needed some tweaks...]flock().For
fstat()the best way to mitigate this is by extracting the actual part populating the array into a separate function which is exported so that SPL can call that.