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

Add PHP client #10

Merged
merged 24 commits into from
Jun 4, 2018
Merged

Add PHP client #10

merged 24 commits into from
Jun 4, 2018

Conversation

stevenay
Copy link
Contributor

@stevenay stevenay commented May 29, 2018

I've been implementing PHP client for myanmar-tools. It is fully tested and already built on travis.

PHP client is targeted to PHP ver 7.0 as minimum because we want type declarations to method arguments and return type. (http://php.net/manual/en/migration70.new-features.php)

As one notice, PHP natural system of logarithms calculation result is a little bit different from the Standard one (e.g. Java version that I took referenced). That's why I've to add very very small delta value (0.00000001) in testing compatibility when implementing test cases.

For the demo code, I haven't added to the samples folder because the composer package for php-client is yet uploaded to packagist. I'm now planning to upload to the packagist after the code has been approved and merged.

Thanks.

image

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@stevenay
Copy link
Contributor Author

I signed it CLA now.

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Collaborator

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Please see my suggestions. Let me know when you have updated your branch.

.gitignore Outdated
.idea
.idea/workspace.xml
.idea/php.xml
clients/php/vendor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please create a new .gitignore file in clients/php/.gitignore and add these two files there, vendor and composer.lock.

For the .idea stuff, you should be able to exclude the whole directory via ".idea/", correct?

.travis.yml Outdated
@@ -60,3 +60,15 @@ matrix:
- bundle install
script:
- rake test

# PHP Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for updating travis.yml. May as well keep it in alphabetical order, though: move this up between JavaScript and Ruby.


for ($i1 = 0; $i1 < $size; $i1++) {
$entries = unpack('n*', fread($stream, 2))[1];
$fallback = ($entries == 0) ? 0.0 : round(unpack("Gnum", fread($stream, 4))['num'], 8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about unpack("G*", $readData)[1] like the other call sites? Also, I don't think you need the round. Could you change this line to:

$fallback = ($entries == 0) ? 0.0 : unpack("G*", fread($stream, 4))[1];


if ($next == $i2) {
$readData = fread($stream, 4);
$logProbabilityDifferences[$i1][$i2] = round(unpack("Gnum", $readData)['num'], 8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here: please simplify the unpack and remove the round.

}

// Deprecated
public static function hex2Float($strHex) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete this?

fclose($inStream);

} catch (Exception $exception) {
throw new Exception("Could not load Markov model from resource file");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change this to

throw new Exception("Could not load Markov model from resource file", 0, $exception);

}

public static function ord_utf8($c)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do this instead:

    public static function ord_utf8($c)
    {
        if (empty($c))
            return;

        $u32 = mb_convert_encoding($c, "UTF-32BE");
        return unpack("N", $u32)[1];
    }

Or, if you are okay adding a dependency on the Intl extension, you can do this:

use IntlChar;

// ...

    public static function ord_utf8($c)
    {
        return IntlChar::ord($c);
    }

$seenTransition = true;
}

$offset += self::charCount($cp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you are using mb_substr, this should be just

$offset += 1;

# Determine the number of char values needed to represent the specified character (Unicode code point)
# if the code point is equal to or greater than 0x10000, then the method returns 2.
# otherwise, the method returns 1.
public static function charCount($codePoint)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is for UTF-16 handling in Java and JavaScript. It is irrelevant in PHP, which is UTF-8. Please delete this function.

$actual = $this->detector->getZawgyiProbability($data[1]);
// Since php natural logarithmic calculation (exp()) is a little bit far from exact value
// we have to put very very small delta point
$delta = 0.00000001;
Copy link
Collaborator

Choose a reason for hiding this comment

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

After you remove the round on line 71 of BinaryMarkov.php, try removing the delta here. That might be the problem. Otherwise, it is fine if you need to keep the delta here.

@stevenay
Copy link
Contributor Author

stevenay commented May 30, 2018

Thanks you so much for your insightful reviews, Shane @sffc . Fixing the code is finished. Let me know if there is something that I should fix.

Thanks.

@stevenay
Copy link
Contributor Author

stevenay commented May 31, 2018

I would like to add a side note about Packagist and Composer.

To upload to Packagist and be downloadable by composer, we have some limitations. Packagist does not support to link to subdirectory of the github repo. Other projects have that problem too. We can see the discussion here.

To solve this problem, I can think of 3 ways according to my knowledge.
The first one is to create another separate repo for PHP version like this Google project does. It only keep Readme file under Php directory in the main repo. It is the best way and easy to maintain in the long run.

The second approach is to move composer.json to the root of the project like this Google project does. But there is one awful downside that the composer will also download all the unnecessary files to the user project. In our case, Java, Ruby, Javascript and other language folders will also be downloaded into the user's PHP project. I think it is a dirty way.

The third approach is to keep another branch for PHP client. In this approach, we will not upload to Packagist. Instead of it, the users have to add repositories definition in their composer.json to link to our github repo like the following.

"repositories": [
        {
            "type": "package",
            "package": {
                "name": "googlei18n/myanmar-tools",
                "version": "v0.0.4",
                "source": {
                    "url": "https://github.com/stevenay/myanmar-tools.git",
                    "type": "git",
                    "reference": "php-client"
                },
                "autoload": {
                    "psr-4": {
                        "Googlei18n\\MyanmarTools\\": "clients/php/src"
                    }
                }
            }
        }
    ],

But in this approach, the composer will not read the composer.json file of the library. As a result, it will not generate autoload for the library. So we have to explicitly add autoload clause in our repositories definition. People also think it is harder to maintain.

Would like to say sorry that to add PHP-Client is a little bothersome for the library maintainers.

@sffc
Copy link
Collaborator

sffc commented Jun 3, 2018

It is unfortunate that Composer has that limitation. This is Composer's problem, and we should not make it our problem.

I lean toward your option 2. Some extra junk will get bundled in the Composer package, but again, that's their problem, not ours, and it might give Composer an incentive to fix their system to allow subdirectory projects. Also, the project is less than 500 KB right now, which really isn't that much overhead. I do not want the extra maintenance cost associated with your options 1 and 3.

You can find much more discussion about the advantages of monorepos here:

www.google.com/search?q=monorepo

@stevenay
Copy link
Contributor Author

stevenay commented Jun 4, 2018

It's ok, @sffc. I agree with your suggestion. So I've updated the repo for this approach:

  1. Move composer.json to the root of the project.
  2. Update travis.yml and Makefile by removing cd statement and add phpunit.xml test config file explicitly in the test command.
  3. Update .gitignore file to reflect updated paths.

@sffc sffc merged commit d7d9266 into google:master Jun 4, 2018
sffc added a commit that referenced this pull request Jun 4, 2018
- Fixing make test for PHP
- composer.lock is intended to be in source control
- Platform-agnostic file separators
@sffc
Copy link
Collaborator

sffc commented Jun 4, 2018

Thanks for the contribution! I merged it and made a few more minor changes in 7624e17 and 0e061f1.

sffc added a commit that referenced this pull request Jul 9, 2018
Adds PHP client. Includes a composer.json file at the top level, required for limitations in Composer's package management system.
sffc added a commit that referenced this pull request Jul 9, 2018
- Fixing make test for PHP
- composer.lock is intended to be in source control
- Platform-agnostic file separators
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.

3 participants