Re: [PHP-DEV] Fixes for parse_url, bug 50563
hi, On Mon, May 24, 2010 at 8:58 PM, Ilia Alshanetsky wrote: > I think Kalle's patch is a really good solution for the trunk. +1 Same here, with a couple of changes like killing the warning (and the related flag) as well as removing the duplicated code. I will do that before applying it. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fixes for parse_url, bug 50563
I think Kalle's patch is a really good solution for the trunk. +1
On Mon, May 24, 2010 at 8:51 AM, Kalle Sommer Nielsen wrote:
> Hey Ralph
>
> 2010/5/21 Ralph Schindler :
> >
> > Hey all,
> >
> > The first patch is against trunk. I think we should at least get this
> done
> > even if the group decides that down the line we want the why portion
> > explained as well (I actually don't care about the why part). That
> feature
> > would require that php_url_parse_ex() add some parsing intellignce, this
> > would not be a 1 or 2 line feature implementation.
>
> I did a quick and dirty patch to turn the $component into a bitfield
> allowing you to do:
> $url = parse_url('http://www.php.net/manual/', PHP_URL_HOST |
> PHP_URL_PATH);
> printf('%s%s', $url['host'], $url['path']);
>
> At the same point I figured we could disable the warning and therefore
> I added a new constant named PHP_URL_SILENT:
> $broken_url = 'http:///www.php.net/';
> var_dump(parse_url($broken_url), parse_url($broken_url, PHP_URL_SILENT));
>
> It doesn't alter the actual URL parser code to tell why the parsing
> failed, but it kills two flies in one hit. Ofcourse the silent option
> can be skipped, but while atleast updating parse_url().
>
> Patch available at, this is a rough patch and does not fix the broken
> tests:
> http://pastie.org/974449
>
> Theres a minor BC break, since it changes the values of the constants,
> but it can be fixed by changing the checking code, or the dirty way to
> increase the values so they don't conflict with the old ones.
>
> --
> regards,
>
> Kalle Sommer Nielsen
> ka...@php.net
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
Re: [PHP-DEV] Fixes for parse_url, bug 50563
Hey Ralph
2010/5/21 Ralph Schindler :
>
> Hey all,
>
> The first patch is against trunk. I think we should at least get this done
> even if the group decides that down the line we want the why portion
> explained as well (I actually don't care about the why part). That feature
> would require that php_url_parse_ex() add some parsing intellignce, this
> would not be a 1 or 2 line feature implementation.
I did a quick and dirty patch to turn the $component into a bitfield
allowing you to do:
$url = parse_url('http://www.php.net/manual/', PHP_URL_HOST | PHP_URL_PATH);
printf('%s%s', $url['host'], $url['path']);
At the same point I figured we could disable the warning and therefore
I added a new constant named PHP_URL_SILENT:
$broken_url = 'http:///www.php.net/';
var_dump(parse_url($broken_url), parse_url($broken_url, PHP_URL_SILENT));
It doesn't alter the actual URL parser code to tell why the parsing
failed, but it kills two flies in one hit. Ofcourse the silent option
can be skipped, but while atleast updating parse_url().
Patch available at, this is a rough patch and does not fix the broken tests:
http://pastie.org/974449
Theres a minor BC break, since it changes the values of the constants,
but it can be fixed by changing the checking code, or the dirty way to
increase the values so they don't conflict with the old ones.
--
regards,
Kalle Sommer Nielsen
ka...@php.net
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fixes for parse_url, bug 50563
hi, I will apply the patch to trunk later today as it seems that we have no objection for the patch itself. The proposal to add more information on failure can be implemented later, if someone fills motivated enough to do it. Cheers, -- Pierre On Sat, May 22, 2010 at 5:28 PM, Pierre Joye wrote: > hi, > > On Sat, May 22, 2010 at 4:42 PM, Ilia Alshanetsky wrote: >> Instead of removing a warning, why not add an additional parameter to the >> function that would instruct it to silence warning messages on parse >> failure? > > What are the actual usefulness of these warnings? I see absolutely no > need to keep them. > > However, it would be useful to have an optional parameter to get why > an URL actually failed. But that can be done later. > > Cheers, > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fixes for parse_url, bug 50563
On Mon, May 24, 2010 at 2:12 PM, Brian Moon wrote: > Because that is, IMO, a bad precedent to start for PHP internal functions. > Too many functions already produce warnings (fopen) and return a status that > can be checked. The solution right now is @ and that has so much baggage > with it that you can now disable that feature completely making distributed > software not be able to deal with these functions and return useful messages > instead of spewing warnings uncontrollably. I am all for turning off > warnings when a function returns a success/failure status. And when this success/failure status is actually being used in the calling code I assume? ;-) > > Brian. > > http://brian.moonspot.net/ > > On 5/22/10 9:42 AM, Ilia Alshanetsky wrote: >> >> Instead of removing a warning, why not add an additional parameter to the >> function that would instruct it to silence warning messages on parse >> failure? >> >> On Fri, May 21, 2010 at 11:55 AM, Brian Moon wrote: >> >>> +1 >>> >>> >>> Brian. >>> >>> http://brian.moonspot.net/ >>> >>> >>> On 5/21/10 10:38 AM, Ralph Schindler wrote: >>> Hey all, Attached is a patch to remove the warning from parse_url() in situations where parse_url() cannot actually parse the url. The bug report also claims there should be a new feature for understanding "why" a parsed url failed. That code currently does not exist, and the current warning gives no more information than simply returning false does. http://bugs.php.net/bug.php?id=50563 The first patch is against trunk. I think we should at least get this done even if the group decides that down the line we want the why portion explained as well (I actually don't care about the why part). That feature would require that php_url_parse_ex() add some parsing intellignce, this would not be a 1 or 2 line feature implementation. The motivation is that since false and E_WARN are redunant, this patch would allow developers to STOP using @parse_url(), which we have to do. I also suggest that we apply this to the PHP_5_3 branch. This behavior should be backwards compatible with everyone that is actually using the @parse_url() in their code. The only 1-off situation that might be affected is if people are catching the error with an error handler and branching there. Why they wouldn't be branching off the false return value, I don't know. Both patches contain test fixes. Is there anything more I need? Is this commit worthy? Thanks, Ralph >>> -- >>> PHP Internals - PHP Runtime Development Mailing List >>> To unsubscribe, visit: http://www.php.net/unsub.php >>> >>> >> > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > -- -- Tjerk -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fixes for parse_url, bug 50563
Because that is, IMO, a bad precedent to start for PHP internal functions. Too many functions already produce warnings (fopen) and return a status that can be checked. The solution right now is @ and that has so much baggage with it that you can now disable that feature completely making distributed software not be able to deal with these functions and return useful messages instead of spewing warnings uncontrollably. I am all for turning off warnings when a function returns a success/failure status. Brian. http://brian.moonspot.net/ On 5/22/10 9:42 AM, Ilia Alshanetsky wrote: Instead of removing a warning, why not add an additional parameter to the function that would instruct it to silence warning messages on parse failure? On Fri, May 21, 2010 at 11:55 AM, Brian Moon wrote: +1 Brian. http://brian.moonspot.net/ On 5/21/10 10:38 AM, Ralph Schindler wrote: Hey all, Attached is a patch to remove the warning from parse_url() in situations where parse_url() cannot actually parse the url. The bug report also claims there should be a new feature for understanding "why" a parsed url failed. That code currently does not exist, and the current warning gives no more information than simply returning false does. http://bugs.php.net/bug.php?id=50563 The first patch is against trunk. I think we should at least get this done even if the group decides that down the line we want the why portion explained as well (I actually don't care about the why part). That feature would require that php_url_parse_ex() add some parsing intellignce, this would not be a 1 or 2 line feature implementation. The motivation is that since false and E_WARN are redunant, this patch would allow developers to STOP using @parse_url(), which we have to do. I also suggest that we apply this to the PHP_5_3 branch. This behavior should be backwards compatible with everyone that is actually using the @parse_url() in their code. The only 1-off situation that might be affected is if people are catching the error with an error handler and branching there. Why they wouldn't be branching off the false return value, I don't know. Both patches contain test fixes. Is there anything more I need? Is this commit worthy? Thanks, Ralph -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fixes for parse_url, bug 50563
hi, On Sat, May 22, 2010 at 4:42 PM, Ilia Alshanetsky wrote: > Instead of removing a warning, why not add an additional parameter to the > function that would instruct it to silence warning messages on parse > failure? What are the actual usefulness of these warnings? I see absolutely no need to keep them. However, it would be useful to have an optional parameter to get why an URL actually failed. But that can be done later. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fixes for parse_url, bug 50563
Instead of removing a warning, why not add an additional parameter to the function that would instruct it to silence warning messages on parse failure? On Fri, May 21, 2010 at 11:55 AM, Brian Moon wrote: > +1 > > > Brian. > > http://brian.moonspot.net/ > > > On 5/21/10 10:38 AM, Ralph Schindler wrote: > >> >> Hey all, >> >> Attached is a patch to remove the warning from parse_url() in situations >> where parse_url() cannot actually parse the url. The bug report also >> claims there should be a new feature for understanding "why" a parsed >> url failed. That code currently does not exist, and the current warning >> gives no more information than simply returning false does. >> >> http://bugs.php.net/bug.php?id=50563 >> >> The first patch is against trunk. I think we should at least get this >> done even if the group decides that down the line we want the why >> portion explained as well (I actually don't care about the why part). >> That feature would require that php_url_parse_ex() add some parsing >> intellignce, this would not be a 1 or 2 line feature implementation. >> >> The motivation is that since false and E_WARN are redunant, this patch >> would allow developers to STOP using @parse_url(), which we have to do. >> >> I also suggest that we apply this to the PHP_5_3 branch. This behavior >> should be backwards compatible with everyone that is actually using the >> @parse_url() in their code. The only 1-off situation that might be >> affected is if people are catching the error with an error handler and >> branching there. Why they wouldn't be branching off the false return >> value, I don't know. >> >> Both patches contain test fixes. >> >> Is there anything more I need? Is this commit worthy? >> >> Thanks, >> Ralph >> >> > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
Re: [PHP-DEV] Fixes for parse_url, bug 50563
+1 Brian. http://brian.moonspot.net/ On 5/21/10 10:38 AM, Ralph Schindler wrote: Hey all, Attached is a patch to remove the warning from parse_url() in situations where parse_url() cannot actually parse the url. The bug report also claims there should be a new feature for understanding "why" a parsed url failed. That code currently does not exist, and the current warning gives no more information than simply returning false does. http://bugs.php.net/bug.php?id=50563 The first patch is against trunk. I think we should at least get this done even if the group decides that down the line we want the why portion explained as well (I actually don't care about the why part). That feature would require that php_url_parse_ex() add some parsing intellignce, this would not be a 1 or 2 line feature implementation. The motivation is that since false and E_WARN are redunant, this patch would allow developers to STOP using @parse_url(), which we have to do. I also suggest that we apply this to the PHP_5_3 branch. This behavior should be backwards compatible with everyone that is actually using the @parse_url() in their code. The only 1-off situation that might be affected is if people are catching the error with an error handler and branching there. Why they wouldn't be branching off the false return value, I don't know. Both patches contain test fixes. Is there anything more I need? Is this commit worthy? Thanks, Ralph -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fixes for parse_url, bug 50563
Really attached this time. Attached is a patch to remove the warning from parse_url() in situations diff --git a/ext/standard/tests/url/parse_url_basic_001.phpt b/ext/standard/tests/url/parse_url_basic_001.phpt index 3d50689..7b9d513 100644 --- a/ext/standard/tests/url/parse_url_basic_001.phpt +++ b/ext/standard/tests/url/parse_url_basic_001.phpt @@ -845,55 +845,29 @@ echo "Done"; string(1) "/" } ---> http:///blah.com: -Warning: parse_url(http:///blah.com): Unable to parse URL in %s on line 15 -bool(false) +--> http:///blah.com: bool(false) ---> http://:80: -Warning: parse_url(http://:80): Unable to parse URL in %s on line 15 -bool(false) +--> http://:80: bool(false) ---> http://user@:80: -Warning: parse_url(http://user@:80): Unable to parse URL in %s on line 15 -bool(false) +--> http://user@:80: bool(false) ---> http://user:pass@:80: -Warning: parse_url(http://user:pass@:80): Unable to parse URL in %s on line 15 -bool(false) +--> http://user:pass@:80: bool(false) ---> http://:: -Warning: parse_url(http://:): Unable to parse URL in %s on line 15 -bool(false) +--> http://:: bool(false) ---> http://@/: -Warning: parse_url(http://@/): Unable to parse URL in %s on line 15 -bool(false) +--> http://@/: bool(false) ---> http://@:/: -Warning: parse_url(http://@:/): Unable to parse URL in %s on line 15 -bool(false) +--> http://@:/: bool(false) ---> http://:/: -Warning: parse_url(http://:/): Unable to parse URL in %s on line 15 -bool(false) +--> http://:/: bool(false) ---> http://?: -Warning: parse_url(http://?): Unable to parse URL in %s on line 15 -bool(false) +--> http://?: bool(false) ---> http://?:: -Warning: parse_url(http://?:): Unable to parse URL in %s on line 15 -bool(false) +--> http://?:: bool(false) ---> http://:?: -Warning: parse_url(http://:?): Unable to parse URL in %s on line 15 -bool(false) +--> http://:?: bool(false) ---> http://blah.com:123456: -Warning: parse_url(http://blah.com:123456): Unable to parse URL in %s on line 15 -bool(false) +--> http://blah.com:123456: bool(false) ---> http://blah.com:abcdef: -Warning: parse_url(http://blah.com:abcdef): Unable to parse URL in %s on line 15 -bool(false) +--> http://blah.com:abcdef: bool(false) Done \ No newline at end of file diff --git a/ext/standard/tests/url/parse_url_basic_002.phpt b/ext/standard/tests/url/parse_url_basic_002.phpt index e25ab8d..f3ac770 100644 --- a/ext/standard/tests/url/parse_url_basic_002.phpt +++ b/ext/standard/tests/url/parse_url_basic_002.phpt @@ -109,43 +109,17 @@ echo "Done"; --> http://[x:80]/ : string(4) "http" -->: NULL --> / : NULL ---> http:///blah.com : -Warning: parse_url(http:///blah.com): Unable to parse URL in %s on line 15 -bool(false) ---> http://:80 : -Warning: parse_url(http://:80): Unable to parse URL in %s on line 15 -bool(false) ---> http://user@:80 : -Warning: parse_url(http://user@:80): Unable to parse URL in %s on line 15 -bool(false) ---> http://user:pass@:80 : -Warning: parse_url(http://user:pass@:80): Unable to parse URL in %s on line 15 -bool(false) ---> http://: : -Warning: parse_url(http://:): Unable to parse URL in %s on line 15 -bool(false) ---> http://@/ : -Warning: parse_url(http://@/): Unable to parse URL in %s on line 15 -bool(false) ---> http://@:/ : -Warning: parse_url(http://@:/): Unable to parse URL in %s on line 15 -bool(false) ---> http://:/ : -Warning: parse_url(http://:/): Unable to parse URL in %s on line 15 -bool(false) ---> http://? : -Warning: parse_url(http://?): Unable to parse URL in %s on line 15 -bool(false) ---> http://?: : -Warning: parse_url(http://?:): Unable to parse URL in %s on line 15 -bool(false) ---> http://:? : -Warning: parse_url(http://:?): Unable to parse URL in %s on line 15 -bool(false) ---> http://blah.com:123456 : -Warning: parse_url(http://blah.com:123456): Unable to parse URL in %s on line 15 -bool(false) ---> http://blah.com:abcdef : -Warning: parse_url(http://blah.com:abcdef): Unable to parse URL in %s on line 15 -bool(false) +--> http:///blah.com : bool(false) +--> http://:80 : bool(false) +--> http://user@:80 : bool(false) +--> http://user:pass@:80 : bool(false) +--> http://: : bool(false) +--> http://@/ : bool(false) +--> http://@:/ : bool(false) +--> http://:/ : bool(false) +--> http://? : bool(false) +--> http://?: : bool(false) +--> http://:? : bool(false) +--> http://blah.com:123456 : bool(false) +--> http://blah.com:abcdef : bool(false) Done \ No newline at end of file diff --git a/ext/standard/tests/url/parse_url_basic_003.phpt b/ext/standard/tests/url/parse_url_basic_003.phpt index e34dc2d..dbd9208 100644 --- a/ext/standard/tests/url/parse_url_basic_003.phpt +++ b/ext/standard/tests/url/parse_url_basic_003.phpt @@ -108,43 +108,17 @@ echo "Done"; --> http://[x:80]/ : string(6) "[x:80]" -->: NULL --> / : NULL ---> http:///blah.com : -Warning: parse_url(http:///blah.com): Unable to parse URL in %s on li
[PHP-DEV] Fixes for parse_url, bug 50563
Hey all, Attached is a patch to remove the warning from parse_url() in situations where parse_url() cannot actually parse the url. The bug report also claims there should be a new feature for understanding "why" a parsed url failed. That code currently does not exist, and the current warning gives no more information than simply returning false does. http://bugs.php.net/bug.php?id=50563 The first patch is against trunk. I think we should at least get this done even if the group decides that down the line we want the why portion explained as well (I actually don't care about the why part). That feature would require that php_url_parse_ex() add some parsing intellignce, this would not be a 1 or 2 line feature implementation. The motivation is that since false and E_WARN are redunant, this patch would allow developers to STOP using @parse_url(), which we have to do. I also suggest that we apply this to the PHP_5_3 branch. This behavior should be backwards compatible with everyone that is actually using the @parse_url() in their code. The only 1-off situation that might be affected is if people are catching the error with an error handler and branching there. Why they wouldn't be branching off the false return value, I don't know. Both patches contain test fixes. Is there anything more I need? Is this commit worthy? Thanks, Ralph -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
