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

Parse_str(string,array) semantics #1146

Closed
zhangmuhua opened this issue Oct 9, 2013 · 11 comments
Closed

Parse_str(string,array) semantics #1146

zhangmuhua opened this issue Oct 9, 2013 · 11 comments

Comments

@zhangmuhua
Copy link

i use function parse_str to parse url, but fail with hhvm.

  1. code as follows:
    1

  2. php interpreter print as follows:
    23
    John Adams

  3. but hhvm interpreter print as follows:
    HipHop Notice: Undefined variable: id in */test/test_parse_str.php on line 4

HipHop Notice: Undefined variable: name in */test/test_parse_str.php on line 6

who can help me? how can i use this function in hhvm?

@zhangmuhua
Copy link
Author

i watched the implementation in hhvm:
989 void f_parse_str(CStrRef str, VRefParam arr /* = null */) {
990 arr = Array::Create();
991 HttpProtocol::DecodeParameters(arr, str.data(), str.size());
992 }

@javer
Copy link
Contributor

javer commented Oct 9, 2013

You should build HHVM from source using master HEAD, because this issue was fixed 22 days ago in #1063.

@zhangmuhua
Copy link
Author

very thx, @javer

@zhangmuhua
Copy link
Author

1)resolved this problem with new version.
2)but there still a bug.
for example:
1

$name is undefined in hhvm.

@javer
Copy link
Contributor

javer commented Oct 9, 2013

It is the same as in PHP. Look at PHP manual http://php.net/manual/en/function.parse-str.php:

Parses str as if it were the query string passed via a URL and sets variables in the current scope.
If the second parameter arr is present, variables are stored in this variable as array elements instead.

@huzhiguang
Copy link
Contributor

@javer ,I have fixed @zhangmuhua 's problem.
This code as follow:

  void f_parse_str(CStrRef str, VRefParam arr /* = null */) {
  Variant result;
    HttpProtocol::DecodeParameters(result, str.data(), str.size());
    if(!arr.isReferenced()) {
            f_extract(result.toArray());
            return;
    }
   ///////////////////////////////fixed notice variable undefined  //////////////////////////
    Array vars=result.toArray();
    Array newVars=Array::Create();
    for (ArrayIter iter(vars); iter; ++iter) {
            Variant key = iter.first();
            Variant value;
            value.setNull();
            newVars.set(key,Variant(value));
    }
    f_extract(newVars);
   ///////////////////////////////////////////////////////////////////////////////
    arr = result.toArray();
   }

I will pull request hiphop-php master.^_^

@javer
Copy link
Contributor

javer commented Oct 9, 2013

@huzhiguang, not sure if it should be fixed, because PHP and HHVM results is the same.

test.php:

<?php
error_reporting(E_ALL);
parse_str("id=23&name=John%20Adams", $a);
print_r($a);
echo $name . "\n";

PHP:

$ php -n test.php
Array
(
    [id] => 23
    [name] => John Adams
)

Notice: Undefined variable: name in test.php on line 5

HHVM:

$ hhvm test.php
Array
(
    [id] => 23
    [name] => John Adams
)
HipHop Notice: Undefined variable: name in test.php on line 5

In your patch you can overwrite previously defined variables, check this test:

<?php
$name = 'Smith';
parse_str("id=23&name=John%20Adams", $a);
echo $name . "\n";

And it will be undocumented behavior because according to PHP manual when second parameter is given no variables should be extracted.

@huzhiguang
Copy link
Contributor

I have fixed this problem, you can try it,do you see any other questions?

fixed code as follow:

  void f_parse_str(CStrRef str, VRefParam arr /* = null */) {
   Variant result;

    HttpProtocol::DecodeParameters(result, str.data(), str.size());

    if(!arr.isReferenced()) {
            f_extract(result.toArray());
            return;
    }
    ///////////////////////////////fixed notice variable undefined  //////////////////////////
    Array vars=result.toArray();
    Array newVars=Array::Create();
    Array allVars=f_get_defined_vars();
    for (ArrayIter iter(vars); iter; ++iter) {
            Variant key = iter.first();
            if(!allVars.exists(key)){
                    Variant value;
                    value.setNull();
                    newVars.set(key,Variant(value));
            }
    }
    f_extract(newVars);
      ///////////////////////////////////////////////////////////////////////////////
    arr = result.toArray();
    }

@javer
Copy link
Contributor

javer commented Oct 9, 2013

@huzhiguang, compare output of this script on PHP and your patched HHVM:

<?php
parse_str("id=23&name=John%20Adams", $a);
var_dump(isset($name));

PHP Output:

bool(false)

As I said before, this is because no variables should be extracted when second parameter of parse_str is given. See https://github.com/php/php-src/blob/master/ext/standard/string.c#L4454-4460 for details.

@scannell
Copy link
Contributor

scannell commented Oct 9, 2013

Thanks @javer.

@zhangmuhua, we're not going to make this work when Zend doesn't -- I suggest you take it up with them if you want the behavior of the language to be changed in 5.6 or 5.7, then we'll consider fixing it.

@scannell scannell closed this as completed Oct 9, 2013
@huzhiguang
Copy link
Contributor

ok,@javer,I have looked at the zend implementation logic, arrayArg !=NULL parse variable don't use zend_rebuild_symbol_table,so variable do not exist,I have understanded.thanks,you provide way is true,although the notice problems.

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

No branches or pull requests

4 participants