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

Fix PHP byte validation and reenable builds #7670

Merged
merged 5 commits into from Nov 29, 2022

Conversation

lsl
Copy link
Contributor

@lsl lsl commented Nov 25, 2022

CI builds for PHP were disabled in #6381 and sometime later removed.

This PR fixes the byte validation error and re-introduces the (updated) build.

The byte validation error was caused by attempting to check if:

0 <= "\0" && "\0" <= 255 // false

coming from this line, which evaluates false, instead validating on the ord value for bytes we can compare like for like:

0 <= ord("\0") && ord("\0") <= 255 // true

which allows the value to be treated as a valid, in range, byte.

Test's also needed a couple of fixes to pass but they looked like bugs.

Testing:

cd tests
php phpTest.php
FlatBuffers php test: completed successfully
flatc --php -o php union_vector/union_vector.fbs
php phpUnionVectorTest.php 

@github-actions github-actions bot added CI Continuous Integration php labels Nov 25, 2022
dbaileychess
dbaileychess previously approved these changes Nov 29, 2022
.github/workflows/build.yml Outdated Show resolved Hide resolved
@dbaileychess dbaileychess merged commit cf89d1e into google:master Nov 29, 2022
sunwen18 pushed a commit to sunwen18/flatbuffers that referenced this pull request Dec 25, 2022
* Fix PHP byte validation and reenable builds

* Use checkout@v3

Co-authored-by: Derek Bailey <derekbailey@google.com>
candhyan pushed a commit to mediaz/flatbuffers that referenced this pull request Jan 2, 2023
* Fix PHP byte validation and reenable builds

* Use checkout@v3

Co-authored-by: Derek Bailey <derekbailey@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration php
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants