Hi Marcus,
FYI, this change:
http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?view=diff&r1=1.146&r2=1.147
breaks about 20 tests in phar, it's a potentially serious BC break. I
understand the reasoning behind it, but you may find other users up in
arms. The main problem is that this line:
flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_SELF;
returns the DirectoryIterator object as current instead of an
SplFileInfo as it used to return. If you instead use:
flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_FILEINFO;
this will at least keep the current behavior of a SplFileInfo class
being returned.
Perhaps the better course of action would be to correct the
documentation rather than the behavior? People upgrading from 5.2.5 to
5.2.6 will have a nasty shock if they relied upon the default
constructor parameters, and even if it is reverted in 5.2.x and kept in
5.3.x, the same problem holds.
In other words, the best fix for this is to change the default value of
$flags in the documentation of the constructor, not to change the
behavior to match faulty docs. This is especially true since the value
of the constant CURRENT_AS_FILEINFO was (incorrectly) set to 0 in the
SPL module startup instead of the expected value - technically, the
documentation (which says $flags = 0) is really saying $flags =
CURRENT_AS_FILEINFO|KEY_AS_PATHNAME because it wasn't until this commit
(http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?r1=1.146&r2=1.147)
that you changed those values to non-zero. The fact that the constant
was changed to the actual expected/documented value does not change the
fact that it's not a good reason to break BC.
Greg
Gregory Beaver wrote:
Hi Marcus,
FYI, this change:
http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?view=diff&r1=1.146&r2=1.147
breaks about 20 tests in phar, it's a potentially serious BC break. I
understand the reasoning behind it, but you may find other users up in
arms. The main problem is that this line:flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_SELF;
Important correction: the offending line is actually:
flags = 0;
in the previous if() {} block.
returns the DirectoryIterator object as current instead of an
SplFileInfo as it used to return. If you instead use:flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_FILEINFO;
this will at least keep the current behavior of a SplFileInfo class
being returned.Perhaps the better course of action would be to correct the
documentation rather than the behavior? People upgrading from 5.2.5 to
5.2.6 will have a nasty shock if they relied upon the default
constructor parameters, and even if it is reverted in 5.2.x and kept in
5.3.x, the same problem holds.In other words, the best fix for this is to change the default value of
$flags in the documentation of the constructor, not to change the
behavior to match faulty docs. This is especially true since the value
of the constant CURRENT_AS_FILEINFO was (incorrectly) set to 0 in the
SPL module startup instead of the expected value - technically, the
documentation (which says $flags = 0) is really saying $flags =
CURRENT_AS_FILEINFO|KEY_AS_PATHNAME because it wasn't until this commit
(http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?r1=1.146&r2=1.147)
that you changed those values to non-zero. The fact that the constant
was changed to the actual expected/documented value does not change the
fact that it's not a good reason to break BC.Greg
Hello Gregory,
hmm, and i thought i was doing it correctly now. So I need to
reinvestigate things. Thanks fo rthe heads up.
marcus
Monday, February 11, 2008, 6:57:09 AM, you wrote:
Gregory Beaver wrote:
Hi Marcus,
FYI, this change:
http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?view=diff&r1=1.146&r2=1.147
breaks about 20 tests in phar, it's a potentially serious BC break. I
understand the reasoning behind it, but you may find other users up in
arms. The main problem is that this line:flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_SELF;
Important correction: the offending line is actually:
flags = 0;
in the previous if() {} block.
returns the DirectoryIterator object as current instead of an
SplFileInfo as it used to return. If you instead use:flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_FILEINFO;
this will at least keep the current behavior of a SplFileInfo class
being returned.Perhaps the better course of action would be to correct the
documentation rather than the behavior? People upgrading from 5.2.5 to
5.2.6 will have a nasty shock if they relied upon the default
constructor parameters, and even if it is reverted in 5.2.x and kept in
5.3.x, the same problem holds.In other words, the best fix for this is to change the default value of
$flags in the documentation of the constructor, not to change the
behavior to match faulty docs. This is especially true since the value
of the constant CURRENT_AS_FILEINFO was (incorrectly) set to 0 in the
SPL module startup instead of the expected value - technically, the
documentation (which says $flags = 0) is really saying $flags =
CURRENT_AS_FILEINFO|KEY_AS_PATHNAME because it wasn't until this commit
(http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?r1=1.146&r2=1.147)
that you changed those values to non-zero. The fact that the constant
was changed to the actual expected/documented value does not change the
fact that it's not a good reason to break BC.Greg
Best regards,
Marcus
Hello Gregory,
It is fixed now.
marcus
Monday, February 11, 2008, 6:57:09 AM, you wrote:
Gregory Beaver wrote:
Hi Marcus,
FYI, this change:
http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?view=diff&r1=1.146&r2=1.147
breaks about 20 tests in phar, it's a potentially serious BC break. I
understand the reasoning behind it, but you may find other users up in
arms. The main problem is that this line:flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_SELF;
Important correction: the offending line is actually:
flags = 0;
in the previous if() {} block.
returns the DirectoryIterator object as current instead of an
SplFileInfo as it used to return. If you instead use:flags = SPL_FILE_DIR_KEY_AS_PATHNAME|SPL_FILE_DIR_CURRENT_AS_FILEINFO;
this will at least keep the current behavior of a SplFileInfo class
being returned.Perhaps the better course of action would be to correct the
documentation rather than the behavior? People upgrading from 5.2.5 to
5.2.6 will have a nasty shock if they relied upon the default
constructor parameters, and even if it is reverted in 5.2.x and kept in
5.3.x, the same problem holds.In other words, the best fix for this is to change the default value of
$flags in the documentation of the constructor, not to change the
behavior to match faulty docs. This is especially true since the value
of the constant CURRENT_AS_FILEINFO was (incorrectly) set to 0 in the
SPL module startup instead of the expected value - technically, the
documentation (which says $flags = 0) is really saying $flags =
CURRENT_AS_FILEINFO|KEY_AS_PATHNAME because it wasn't until this commit
(http://cvs.php.net/viewvc.cgi/php-src/ext/spl/spl_directory.c?r1=1.146&r2=1.147)
that you changed those values to non-zero. The fact that the constant
was changed to the actual expected/documented value does not change the
fact that it's not a good reason to break BC.Greg
Best regards,
Marcus