Skip to content

Fix #16 - gzopen() / gzopen64() - 32 bit builds of Ubuntu 14.04 #17

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

Merged
merged 1 commit into from
Dec 18, 2015
Merged

Fix #16 - gzopen() / gzopen64() - 32 bit builds of Ubuntu 14.04 #17

merged 1 commit into from
Dec 18, 2015

Conversation

fisharebest
Copy link
Contributor

Fix for #16

function gzseek($zp , $offset, $whence = SEEK_SET) { return gzseek64($zp, $offset, $whence); }
}
if (!function_exists('gztell') && function_exists('gztell64')) {
function gztell($zp) { return gztell64($zp); }
Copy link
Member

Choose a reason for hiding this comment

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

return p\Php56::gztell64($zp); ?

Copy link
Member

Choose a reason for hiding this comment

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

i'm ok with not duplicating the function as static method in the Php56 class (reduces boiler plates)

Copy link
Member

Choose a reason for hiding this comment

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

why that ?

Copy link
Member

Choose a reason for hiding this comment

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

more consistency and easier to test when moved into Php56

Copy link
Member

Choose a reason for hiding this comment

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

I wonder @keradus how would you test that? We'd need the broken ubuntu isn't it?

@keradus
Copy link
Member

keradus commented Dec 7, 2015

adding some tests would be awesome!

@fisharebest
Copy link
Contributor Author

The travis failure is not in my code ;-)

It looks like php-7 is using phpunit-5, but perhaps the tests were written for phpunit-4

@keradus
Copy link
Member

keradus commented Dec 7, 2015

ref #15 ( and #15 (comment) )

// Missing functions in PHP 5.5.9 - affects 32 bit builds of Ubuntu 14.04LTS
// See https://bugs.launchpad.net/ubuntu/+source/php5/+bug/1315888
if (!function_exists('gzopen') && function_exists('gzopen64')) {
function gzopen($filename , $mode = 'r', $use_include_path = 0 ) { return gzopen64($filename, $mode, $use_include_path); }
Copy link
Member

Choose a reason for hiding this comment

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

$mode doesn't default to 'r' per the doc (http://php.net/gzopen)
trailing space before )

Copy link
Member

Choose a reason for hiding this comment

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

and shouldn't $use_include_path be a boolean instead ?

Copy link
Member

Choose a reason for hiding this comment

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

ah no, they use a crappy int API instead of a proper boolean

@stof
Copy link
Member

stof commented Dec 7, 2015

The Travis failure is because of a BC break in PHPUnit 5.1

@fisharebest
Copy link
Contributor Author

adding some tests would be awesome!

Since we are working around a platform-specific problem, I'm not sure how we create a meaningful test?

To trigger the code, we'd need to create a situation where the function gzopen() does not exist. Since we cannot "unload" PHP libraries at runtime, I don't see how to do this.

Besides, none of the other "thin wrappers" have unit tests.

FYI, I've tested the code on a VM...

@fisharebest
Copy link
Contributor Author

$mode doesn't default to 'r'

Oops. My bad. Fixed

function gzopen($filename , $mode, $use_include_path = 0) { return gzopen64($filename, $mode, $use_include_path); }
}
if (!function_exists('gzseek') && function_exists('gzseek64')) {
function gzseek($zp , $offset, $whence = SEEK_SET) { return gzseek64($zp, $offset, $whence); }
Copy link
Member

Choose a reason for hiding this comment

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

extra space before first comma

@nicolas-grekas
Copy link
Member

👍

@nicolas-grekas
Copy link
Member

Thank you @fisharebest.

@nicolas-grekas nicolas-grekas merged commit 485a564 into symfony:master Dec 18, 2015
nicolas-grekas added a commit that referenced this pull request Dec 18, 2015
… 14.04 (fisharebest)

This PR was merged into the 1.0-dev branch.

Discussion
----------

Fix #16 - gzopen() / gzopen64() - 32 bit builds of Ubuntu 14.04

Fix for #16

Commits
-------

485a564 Fix #16 - gzopen() / gzopen64() - 32 bit builds of Ubuntu 14.04
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 10, 2016
fabpot added a commit that referenced this pull request Mar 10, 2016
This PR was merged into the 1.1-dev branch.

Discussion
----------

Add CHANGELOG

Fixes #32

* v1.1.1

 * bug #40 [Apcu] Load APCUIterator only when APCIterator exists (nicolas-grekas)
 * bug #37 [Iconv] Fix wrong use in bootstrap.php (tucksaun)
 * bug #31 Fix class_uses polyfill (WouterJ)

* v1.1.0

 * feature #22 [APCu] A new polyfill for the legacy APC users (nicolas-grekas)
 * bug #28 [Php70] Workaround https://bugs.php.net/63206 (nicolas-grekas)

* v1.0.1

 * bug #14 ldap_escape does not encode leading/trailing spaces. (ChadSikorra)
 * bug #17 Fix #16 - gzopen() / gzopen64() - 32 bit builds of Ubuntu 14.04 (fisharebest)

* v1.0.0

 * Hello symfony/polyfill

Commits
-------

1781007 Add CHANGELOG
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.

4 participants