Skip to content
This repository has been archived by the owner on Mar 15, 2020. It is now read-only.

Added support for PHP 5.3.3 #8

Merged
merged 2 commits into from
May 28, 2015
Merged

Conversation

webmozart
Copy link
Contributor

This branch replaces the short by the long array notation to add support for PHP 5.3.3.

@GrahamCampbell
Copy link

👎

@GrahamCampbell
Copy link

I really don't want to see new libraries supporting EOL php. :(

@webmozart
Copy link
Contributor Author

@GrahamCampbell That depends largely on the target audience. For very low-level libraries with a large target audience, support for 5.3 is unfortunately still mandatory.

For high-level libs or frameworks, that's a different story, of course..

@stof
Copy link
Contributor

stof commented May 28, 2015

@GrahamCampbell not supporting PHP 5.3 in this library means that any tools which wants to keep support for 5.3 would have to implement phar-updating themselves rather than using this secure implementation.
So supporting 5.3 here is a good idea

@GrahamCampbell
Copy link

Or it just forces them off php 5.3...

@@ -5,6 +5,7 @@ php:
- 5.6
- 5.5
- 5.4
- 5.3.3
Copy link
Contributor

Choose a reason for hiding this comment

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

you should test on 5.3 rather than 5.3.3. The 5.3.3 image on Travis does nont have OpenSSL

Choose a reason for hiding this comment

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

The point of this was to prove that it works on php 5.3.3 and not just a much later version of 5.3 though isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

well, then it should be tested on both. Hopefully people still using 5.3 are at least on a recent 5.3 version

@stof
Copy link
Contributor

stof commented May 28, 2015

@padraic tests checking that phars signed with openssl are updated should be skipped if the system running the tests does not have Openssl

@webmozart
Copy link
Contributor Author

@GrahamCampbell You can do that if you change the min version for large and established projects (ZF, Symfony, Typo3, ..). New and yet unused libraries won't force anyone.

@GrahamCampbell
Copy link

@webmozart Symfony is moving forward though. 2.7 needs more than just php 5.3.3 now, and 3.0 will need 5.5.9+.

@stof
Copy link
Contributor

stof commented May 28, 2015

@GrahamCampbell sure. but phar-updater does not have the same power. If it requires 5.4+, projects allowing 5.3 today will just avoid switching to phar-updater

@webmozart
Copy link
Contributor Author

I added 5.3 to travis.yml and marked the tests as skipped now that fail if openssl is not installed. Travis is green now.

if (!extension_loaded('openssl')) {
$this->markTestSkipped('This test requires the openssl extension to run.');

return;
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to return here (markTestSkipped actually throw an exception to stop the test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

really? :) I never cared to look inside, good to know

padraic added a commit that referenced this pull request May 28, 2015
Added support for PHP 5.3.3
@padraic padraic merged commit 0ee989e into humbug:master May 28, 2015
@padraic
Copy link
Collaborator

padraic commented May 28, 2015

I'd prefer to see folk adopt secure updates, and the reality is that phar-updater is genuinely too small with the sole 5.4 feature used being short array syntax. 5.4 adoption is better driven by larger units of functionality. I already have Humbug and Mockery at 5.4, so compromise is fine by me on this significantly smaller focused package.

@padraic
Copy link
Collaborator

padraic commented May 28, 2015

I'll tag a new release in a few hours @webmozart.

@webmozart
Copy link
Contributor Author

Thanks a lot, much appreciated :)

@padraic
Copy link
Collaborator

padraic commented May 28, 2015

And tagged.

@webmozart
Copy link
Contributor Author

Awesome! 👏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants