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

[Translation] Make IcuDatFileLoader/IcuResFileLoader::load invalid resource compatible with HHVM. #10697

Closed
wants to merge 3 commits into from

Conversation

idn2104
Copy link
Contributor

@idn2104 idn2104 commented Apr 12, 2014

[Translation] HHVM throws when an invalid ResourceBundle is constructed, while zend returns FALSE from the constructor. This patch makes IcuResFileLoader and IcuDatFileLoader compatible with HHVM.

The following tests now pass on HHVM:
https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/Translation/Tests/Loader/IcuDatFileLoaderTest.php#L33
https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/Translation/Tests/Loader/IcuResFileLoaderTest.php#L54

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

$rb = new \ResourceBundle($locale, $resource);
} catch (\Exception $e) {
// HHVM compatibility: constructor throws on invalid resource
$rb = null;
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 false, right?

@fabpot
Copy link
Member

fabpot commented Apr 12, 2014

There are other occurrences of this pattern in src/Symfony/Component/Intl/ResourceBundle/Reader/BinaryBundleReader.php and src/Symfony/Component/Intl/ResourceBundle/Transformer/Rule/LocaleBundleTransformationRule.php

@fabpot
Copy link
Member

fabpot commented Apr 12, 2014

Also, it looks like that it returns null, not false in PHP.

@idn2104
Copy link
Contributor Author

idn2104 commented Apr 12, 2014

Thanks, I meant to write false where I wrote null earlier.

From the documentation it seems it should return false: http://www.php.net/manual/en/resourcebundle.create.php
Returns ResourceBundle object or FALSE on error.

If it does return false, then
src/Symfony/Component/Intl/ResourceBundle/Reader/BinaryBundleReader.php and src/Symfony/Component/Intl/ResourceBundle/Transformer/Rule/LocaleBundleTransformationRule.php
might not handle invalid bundles correctly, since false === null does not hold, and both of these error-check using if (null === $bundle) {. I can't say for sure since I haven't tested it.

@fabpot
Copy link
Member

fabpot commented Apr 12, 2014

I've tested it and it does return null, not false (at least on PHP 5.3 and PHP 5.5 -- so it looks like the documentation is wrong).

@idn2104
Copy link
Contributor Author

idn2104 commented Apr 12, 2014

Confirmed, I've made the changes and updated the other two classes.

Is there a reason to use if (null === $bundle) instead of if (!$bundle)?

@wouterj
Copy link
Member

wouterj commented Apr 12, 2014

Is there a reason to use if (null === $bundle) instead of if (!$bundle)?

We always do a strict comparisation of null.

@fabpot
Copy link
Member

fabpot commented Apr 12, 2014

Thanks @idn2104.

fabpot added a commit that referenced this pull request Apr 12, 2014
… invalid resource compatible with HHVM. (idn2104)

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

Discussion
----------

[Translation] Make IcuDatFileLoader/IcuResFileLoader::load invalid resource compatible with HHVM.

[Translation] HHVM throws when an invalid ResourceBundle is constructed, while zend returns FALSE from the constructor. This patch makes IcuResFileLoader and IcuDatFileLoader compatible with HHVM.

The following tests now pass on HHVM:
https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/Translation/Tests/Loader/IcuDatFileLoaderTest.php#L33
https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/Translation/Tests/Loader/IcuResFileLoaderTest.php#L54

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

Commits
-------

9bc08c0 [Translation] Make IcuDatFileLoader/IcuResFileLoader::load invalid resource compatible with HHVM.
@fabpot fabpot closed this Apr 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants