Skip to content

Conversation

dreamsxin
Copy link
Contributor

No description provided.

@nikic
Copy link
Member

nikic commented Mar 7, 2017

There are test failures on Travis.

/cc @yohgaki

@nikic nikic added the Feature label Mar 7, 2017
@yohgaki
Copy link
Contributor

yohgaki commented Mar 8, 2017

The change is good. I'll look into why travis fails.

@yohgaki
Copy link
Contributor

yohgaki commented Mar 8, 2017

@dreamsxin
You set invalid char for session ID in ext/session/tests/session_id_error3.phpt. Is the test passed on your system?

@dreamsxin
Copy link
Contributor Author

@yohgaki
I will fix, In the editor copy invalid char auto change.

@yohgaki
Copy link
Contributor

yohgaki commented Mar 8, 2017

@dreamsxin
I haven't look into the related code close enough, but IIRC, invalid chars handling is up to save handlers except few chars. All you may have to do is to set appropriate save handler for the test. I guess you are using 3rd party save handler such as memcached.

Invalid chars in session ID is perfectly OK in tests, so you may keep using invalid session ID if you would like. I suggest to use escape sequence in this case.

@dreamsxin
Copy link
Contributor Author

dreamsxin commented Mar 8, 2017

@yohgaki Help see why fails.

@yohgaki
Copy link
Contributor

yohgaki commented Mar 8, 2017

@dreamsxin You don't have to discard invalid chars. Set save handler to "files", and adjust expected test script outputs. Since some developers may have problem with invalid chars, it's better to use escape sequence for invalid chars(bytes). e.g. "\x01"

@dreamsxin
Copy link
Contributor Author

@yohgaki Use hex string? I will replace. But now for the failure now has nothing to do with that.

@dreamsxin
Copy link
Contributor Author

Already used 16 decimal values instead.

@yohgaki
Copy link
Contributor

yohgaki commented Mar 9, 2017

@dreamsxin
It seems test is fixed.
I'm not quite sure how you fixed error, but I guess your test was failed because session.c removes some offending chars for session ID embedding in web pages. I'll check your patch fully and merge later. Thank you for your pull request!

@krakjoe
Copy link
Member

krakjoe commented Jul 25, 2017

@yohgaki what is the status of this patch ?

@yohgaki
Copy link
Contributor

yohgaki commented Jul 25, 2017

@krakjoe
The diff is good to merge. It should prevent changing id, but return current id when name is null. Please merge this.

@krakjoe
Copy link
Member

krakjoe commented Jul 25, 2017

Merged 072ef62

Thanks.

@krakjoe krakjoe closed this Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants