Hi all,
In Phar we have an internal Windows-only function named
phar_unixify_path_separators(), which is needed because Phar is intended to
be used with web apps.
Phar inherits from SPL, which currently uses the system-specific
DEFAULT_SLASH when returning directory paths. This isn't actually a problem
in Phar (at least, as far as I'm aware) but it does look... well, untidy.
The attached patch implements spl_unixify_path_separators(). If it's
accepted, we can probably kill the version in Phar and rely on the
definition in SPL. However I'm concerned that some other extension might be
relying on SPL's current behaviour, unlikely tho' it may seem.
I haven't found any change in behaviour running the test suite for: ext/spl,
ext/phar, ext/standard/array, ext/simplexml. I haven't tested: ext/pdo,
ext/sqlite, ext/mysqli or pecl/spl_types, and I'm not certain if there
should be any more on that list.
If you find/can see any problems with this change at all, pls adv. If there
are no objections, I plan to commit at the end of this week.
Thanks,
- Steph
Hi!
The attached patch implements spl_unixify_path_separators(). If it's
I'm afraid that automatically applying slash conversions to all results
returned by SPL file functions - if that's what this patch does - may be
unwanted. This way SPL functions and regular file functions would return
different file names, which can be harmful if these names are used as
keys, compared, etc.
--
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
The attached patch implements spl_unixify_path_separators(). If it's
I'm afraid that automatically applying slash conversions to all results
returned by SPL file functions - if that's what this patch does - may be
unwanted. This way SPL functions and regular file functions would return
different file names, which can be harmful if these names are used as
keys, compared, etc.
Paths. Mm, possibly.
On the other hand, the only code likely to be broken would be
platform-specific code written for Windows boxes - how much of that is
likely to be out there? Another point is that, although Windows writes
'', these days it's capable of recognising either separator. So it really
would just be down to path comparisons.
I'm actually wondering how much harm it would do to make DEFAULT_SLASH
generic...
- Steph
Hi!
On the other hand, the only code likely to be broken would be
platform-specific code written for Windows boxes - how much of that is
likely to be out there? Another point is that, although Windows writes
Any amount, but I don't see why it would break only windows-specific
code. If I use any other function to get filenames, and/or use
PATH_SEPARATOR to compose names, and then use these names in the same
context as those converting SPL function, I'd get different names on
Windows even if my application was written in most generic way. Do I
miss something?
'', these days it's capable of recognising either separator. So it
really would just be down to path comparisons.
Path comparison is important.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Any amount, but I don't see why it would break only windows-specific
code. If I use any other function to get filenames, and/or use
PATH_SEPARATORto compose names, and then use these names in the same
context as those converting SPL function, I'd get different names on
Windows even if my application was written in most generic way. Do I
miss something?
Doesn't PATH_SEPARATOR use DEFAULT_SLASH?
'', these days it's capable of recognising either separator. So it
really would just be down to path comparisons.Path comparison is important.
I thought so too until I couldn't find any broken tests.
- Steph
--
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
Windows even if my application was written in most generic way. Do I
miss something?Doesn't
PATH_SEPARATORuse DEFAULT_SLASH?
Should be DIRECTORY_SEPARATOR and it's "" on windows (PATH_SEPARATOR is
";" ). Do you propose to change it to "/"?
Path comparison is important.
I thought so too until I couldn't find any broken tests.
With all due respect to phpt, these are very primitive code samples
testing very narrow functional areas. They do not come anywhere near the
stuff people do in real-life apps, combining different functions and
extensions.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi Stas,
Doesn't
PATH_SEPARATORuse DEFAULT_SLASH?Should be
DIRECTORY_SEPARATOR
Bleh, must be nearly switching-off time
and it's "" on windows (PATH_SEPARATOR is ";" ). Do you propose to change
it to "/"?
dirsep_str[0] = DEFAULT_SLASH;
dirsep_str[1] = '\0';
REGISTER_STRING_CONSTANT("DIRECTORY_SEPARATOR", dirsep_str,
CONST_CS|CONST_PERSISTENT);
Path comparison is important.
I thought so too until I couldn't find any broken tests.
With all due respect to phpt, these are very primitive code samples
testing very narrow functional areas. They do not come anywhere near the
stuff people do in real-life apps, combining different functions and
extensions.
Fair point. I like Marcus' idea tho'.
- Steph
--
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
Fair point. I like Marcus' idea tho'.
As long as the default stays the same and doesn't hurt anybody not
touching the option - go for it.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hello Steph,
how about having this as an option inside the SPL classes that gets
turned on by Phar automatically? Inside SPL we could have it as a user
set-only flag.
marcus
Wednesday, June 18, 2008, 10:28:40 PM, you wrote:
Index: ext/spl/spl_directory.c
RCS file: /repository/php-src/ext/spl/spl_directory.c,v
retrieving revision 1.45.2.27.2.23.2.22
diff -u -r1.45.2.27.2.23.2.22 spl_directory.c
--- ext/spl/spl_directory.c 18 Jun 2008 10:05:29 -0000 1.45.2.27.2.23.2.22
+++ ext/spl/spl_directory.c 18 Jun 2008 17:16:39 -0000
@@ -185,6 +185,9 @@
intern->file_name_len =
spprintf(&intern->file_name, 0, "%s%c%s",spl_filesystem_object_get_path(intern,
NULLTSRMLS_CC),
DEFAULT_SLASH, intern->u.dir.entry.d_name);
+#ifdef PHP_WIN32
spl_unixify_path_separators(intern->file_name, intern->file_name_len);+#endif
break;
}
}
@@ -1196,6 +1199,9 @@
subdir->u.dir.sub_path_len = strlen(intern->u.dir.entry.d_name);
subdir->u.dir.sub_path =
estrndup(intern->u.dir.entry.d_name, subdir->u.dir.sub_path_len);
}
+#ifdef PHP_WIN32
spl_unixify_path_separators(intern->u.dir.sub_path, intern->u.dir.sub_path_len);+#endif
subdir->info_class = intern->info_class;
subdir->file_class = intern->file_class;
subdir->oth = intern->oth;
@@ -1227,6 +1233,9 @@if (intern->u.dir.sub_path) { len = spprintf(&sub_name, 0, "%s%c%s",intern->u.dir.sub_path, DEFAULT_SLASH, intern->u.dir.entry.d_name);
+#ifdef PHP_WIN32
spl_unixify_path_separators(sub_name, len);+#endif
RETURN_STRINGL(sub_name, len, 0);
} else {
RETURN_STRING(intern->u.dir.entry.d_name, 1);
Index: ext/spl/spl_directory.hRCS file: /repository/php-src/ext/spl/spl_directory.h,v
retrieving revision 1.12.2.5.2.4.2.10
diff -u -r1.12.2.5.2.4.2.10 spl_directory.h
--- ext/spl/spl_directory.h 20 May 2008 21:46:50 -0000 1.12.2.5.2.4.2.10
+++ ext/spl/spl_directory.h 18 Jun 2008 17:10:18 -0000
@@ -112,6 +112,20 @@
return (spl_filesystem_object*)((char*)it -
XtOffsetOf(spl_filesystem_object, it));
}+#ifdef PHP_WIN32
+static inline void spl_unixify_path_separators(char *path, int path_len)
+{
char *s;/* unixify win paths */for (s = path; s - path < path_len; ++s) {if (*s == '\\') {*s = '/';}}+}
+#endif
#define SPL_FILE_OBJECT_DROP_NEW_LINE 0x00000001 /* drop new lines /
#define SPL_FILE_OBJECT_READ_AHEAD 0x00000002 / read on rewind/next /
#define SPL_FILE_OBJECT_SKIP_EMPTY 0x00000006 / skip empty lines */
Best regards,
Marcus
Hi Marcus,
how about having this as an option inside the SPL classes that gets
turned on by Phar automatically? Inside SPL we could have it as a user
set-only flag.
You are brilliant. I owe you a beer :)
- Steph
Hi Marcus,
how about having this as an option inside the SPL classes that gets
turned on by Phar automatically? Inside SPL we could have it as a user
set-only flag.
The attached patch does this, at the SPL end. Can I commit it (or something
very close) before the 5_3 freeze please?
NB The patch was created with DOS line endings and then converted to *nix,
it won't apply cleanly in its current state. It's just for review.
Thanks,
- Steph
Hello Steph,
Wednesday, July 23, 2008, 10:24:35 PM, you wrote:
Hi Marcus,
how about having this as an option inside the SPL classes that gets
turned on by Phar automatically? Inside SPL we could have it as a user
set-only flag.
The attached patch does this, at the SPL end. Can I commit it (or something
very close) before the 5_3 freeze please?
It's a fix is it not? anyway, please go ahead and submit.
NB The patch was created with DOS line endings and then converted to *nix,
it won't apply cleanly in its current state. It's just for review.
Thanks,
- Steph
Best regards,
Marcus