-
Notifications
You must be signed in to change notification settings - Fork 408
For php7.2 #264
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
For php7.2 #264
Conversation
Sorry, I was not reading the document I will read it now. |
Hi @okashitay , thanks for starting on this PR! One thing I'm a bit unclear about is which changes are 7.2-only and break usage on PHP 7.1. I know the latest Red Hat Enterprise Linux 7 and CentOS 7 are still shipping with PHP 7.1, so while it will be be worth breaking backward compatibility with PHP 5.x soon, I'd rather not break BC with PHP 7.1 unless there are significant demonstrable benefits. |
Hi @adamfranco , |
Hey, firstly, thanks to all the contributors on this project. We've been using this library for years where I work. If you don't mind me asking, what's the status for this PR? Has it been abandoned? We're migrating to PHP 7.2 soon and we're worried that phpCAS might be a blocker. I couldn't find a mention of PHP7+ compatibility in the documentation. If anyone could give me some pointers I'd greatly appreciate. |
Hi @doMynation are you experiencing issues in PHP 7.2? Please see #255 because it seems that compatibility issue has been resolved. |
Thank you for the prompt reply. I am not experiencing issues yet, I was mostly wondering (for my own peace of mind) if PHP 7.2 was officially supported. |
@@ -37,7 +37,12 @@ | |||
* @license http://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0 | |||
* @link https://wiki.jasig.org/display/CASC/phpCAS | |||
*/ | |||
class CAS_Tests_Cas20AttributesTest extends PHPUnit_Framework_TestCase | |||
|
|||
require_once dirname(__FILE__) ."/../../../vendor/autoload.php"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These autoload require
s are not necessary?
@@ -14,7 +14,7 @@ | |||
"ext-curl": "*" | |||
}, | |||
"require-dev": { | |||
"phpunit/phpunit": "~3.7.10" | |||
"phpunit/phpunit": "~6.5.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is PHPUnit 7 better? (PHPUnit 8 only supports PHP7+, which is not good here)
Rather than maintaining If there is, I could give it a shot and it would move the package towards modern PHP community idioms. |
I would say this will need to be eventually implemented at some point in the future, but we just need to retain compatibility somehow..? |
Personally we prefer composer now, and this work is also covered by #332. I am going to close the PR. Thank you for raising the issue and attempting to fix it though! |
Overview
Issue #255
Pull Request OK?
This pull request works with php 7.2 or higher.
Please use ver.1.3.5 for PHP 7.1 and below.