Skip to content

[HttpFoundation] Add a dependency on the mbstring polyfill #18030

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

Closed

Conversation

jakzal
Copy link
Contributor

@jakzal jakzal commented Mar 6, 2016

Q A
Branch 2.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Alternative for #18029.

@jakzal jakzal force-pushed the binary-file-response-mbstring-polyfill branch 2 times, most recently from f96ebb9 to f92756f Compare March 6, 2016 17:00
@@ -36,6 +36,7 @@ public function testConstruction()

public function testConstructWithNonAsciiFilename()
{
var_dump(scandir(__DIR__.'/Fixtures/'));
Copy link
Member

Choose a reason for hiding this comment

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

should be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just debugging. I'm not exactly sure why this fails on windows :)

@jakzal jakzal force-pushed the binary-file-response-mbstring-polyfill branch from f92756f to 40467f7 Compare March 6, 2016 17:15
@jakzal jakzal force-pushed the binary-file-response-mbstring-polyfill branch 4 times, most recently from bff17a0 to 4e51e73 Compare March 6, 2016 17:45
@jakzal
Copy link
Contributor Author

jakzal commented Mar 6, 2016

Tests pass everywhere now. In order for file names with non-ascii characters work on all platforms they need to be created from PHP (just like we do it in FileTest).

Status: needs review

@xabbuh
Copy link
Member

xabbuh commented Mar 6, 2016

We can now also remove the fixtures file that is not used anymore, can't we?

@jakzal
Copy link
Contributor Author

jakzal commented Mar 6, 2016

It's used in the other test.


$this->assertSame('fööö.html', $response->getFile()->getFilename());

@unlink(sys_get_temp_dir().'/fööö.html');
Copy link
Member

Choose a reason for hiding this comment

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

We should remove the file before the assertion. Otherwise it wouldn't be removed when the assertion failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@xabbuh
Copy link
Member

xabbuh commented Mar 6, 2016

Hm indeed, can't we reuse the same approach in that test?

A file with non-ascii characters in the name needs to be created from PHP
in order for it to be read properly on all platforms.
@jakzal jakzal force-pushed the binary-file-response-mbstring-polyfill branch from 4e51e73 to e3eb4f9 Compare March 7, 2016 10:23
@jakzal
Copy link
Contributor Author

jakzal commented Mar 7, 2016

@xabbuh actually, the other test didn't need the file to exist, as only the file name is used there. I removed the fixture.

@jakzal
Copy link
Contributor Author

jakzal commented Mar 7, 2016

@symfony/deciders this PR, when merged, will fix the build

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2016

👍

Status: Reviewed

@nicolas-grekas
Copy link
Member

Thank you @jakzal.

nicolas-grekas added a commit that referenced this pull request Mar 7, 2016
…ll (jakzal)

This PR was squashed before being merged into the 2.3 branch (closes #18030).

Discussion
----------

[HttpFoundation] Add a dependency on the mbstring polyfill

| Q             | A
| ------------- | ---
| Branch        | 2.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? |no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Alternative for #18029.

Commits
-------

59b9f15 [HttpFoundation] Add a dependency on the mbstring polyfill
@jakzal jakzal deleted the binary-file-response-mbstring-polyfill branch March 7, 2016 13:13
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.

5 participants