Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow urlencode(int) #16971

Closed
wants to merge 1 commit into from
Closed

allow urlencode(int) #16971

wants to merge 1 commit into from

Conversation

divinity76
Copy link
Contributor

in strict mode it's kindof annoying to have to cast-to-string when you have a string|int variable you need to urlencode. There is no ambiguity as to how to urlencode integers, so it shouldn't be a problem.

TypeError: urlencode(): Argument #1 ($string) must be of type string, int given

has happened to me enough times now that I bothered to make PR to allow it.

Fwiw the integer path could be micro-optimized to bypass php_raw_url_encode() entirely, at least when the integer is not negative, but for brevity, I didn't bother micro-optimizing it.

in strict mode it's kindof annoying to have to cast-to-string when you have a string|int variable you need to urlencode.
There is no ambiguity as to how to urlencode integers, so it shouldn't be a problem.
"TypeError: urlencode(): Argument php#1 ($string) must be of type string, int given"
Has happened to me enough times now that I bothered to make PR to allow it.

Fwiw the integer path could be micro-optimized to bypass php_raw_url_encode() entirely, at least when the integer is not negative,
but for brewity, I didn't bother micro-optimizing it.
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very ad hoc... If we start doing this, what other function do we have to relax?
url-encoding a number doesn't really make a lot of sense to me. This defeats the point of strict types.
In short: I'm against this change

@divinity76
Copy link
Contributor Author

If we start doing this, what other function do we have to relax?

perhaps escapeshellarg. (i don't recommend escapeshellarg tho)

This defeats the point of strict types.

fwiw comparing PHP to Perl and Javascript:

$ cat test.pl   
use strict;
use warnings;
use URI::Escape;

my $encoded = uri_escape(1);
print "$encoded\n";
$ perl test.pl
1
$ cat test.js
"use strict";
console.log(encodeURIComponent(1));
$ nodejs test.js
1
$ cat test.php
<?php
declare(strict_types=1);
var_dump(urlencode(1));
$ php test.php
PHP Fatal error:  Uncaught TypeError: urlencode(): Argument #1 ($string) must be of type string, int given in /home/hans/test.php:3
Stack trace:
#0 /home/hans/test.php(3): urlencode()
#1 {main}
  thrown in /home/hans/test.php on line 3

both Perl and JS allows it in strict mode 🤔

url-encoding a number doesn't really make a lot of sense to me.

it's when you have a int|string variable and you need to make sure it's safe to include in a url.
my latest encounter was

'url' => '(...)?campaignid=' . urldecode($campaign['campaignid']),

where campaignid could be int|string, so my options were

'url' => '(...)?' . http_build_query(array('campaignid' => $campaign['campaignid']))
'url' => '(...)?campaignid=' . urldecode((string)$campaign['campaignid']),

In short: I'm against this change

that's fair

@divinity76 divinity76 closed this Nov 27, 2024
@nielsdos
Copy link
Member

The strict modes of javascript and perl are unrelated to types, so they're not a fair comparison.

@divinity76
Copy link
Contributor Author

well http_build_query accepts array(1=>1) (and for good reason imo)

@Girgias
Copy link
Member

Girgias commented Nov 28, 2024

I'm not convinced either, so this needs a discussion on internals at the minimum, if not an RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants