-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
[v2.0] Drop support of PHP 5.6, 7.0 and 7.1 #5
[v2.0] Drop support of PHP 5.6, 7.0 and 7.1 #5
Conversation
Update all code base to support PHP 7.1+ only. Add strict type declaration. Add typehints for all parameters and return types. ExceptionInterface now extends Throwable.
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.
LGTM! Is Static analysis already running against this codebase?
How about reducing the BC surface by marking some symbols internal? Like exception constructors? This way, you won’t have to release a major every week :-) |
* @return self | ||
*/ | ||
public static function unableToChangeChmod($file) | ||
public static function unableToChangeChmod(string $file) : self |
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.
This could be made internal to avoid future BC breaks. You'll be able to remove or rename it at any time.
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.
Marked as internal and also changed all exceptions to be not instantiable (private construct).
interface ExceptionInterface | ||
use Throwable; | ||
|
||
interface ExceptionInterface extends Throwable |
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.
Suffixes like Interface
are discouraged by the Doctrine coding standard since they don't carry any meaning from the API standpoint. Would it make sense to rename it to Webimpress\SafeWriter\Exception
?
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.
I am using webimpress/coding-standard
here and it is one of the rules to have Interface
suffix. The same approach we have in zendframework, and it's also my personal preference.
Recently in work we had issue with service called User
and we had also entity called User
. In many places it caused conflict and context was unclear. In most places where we used service we import it with alias UserService
and finally we renamed it, so now all is clear.
composer.json
Outdated
@@ -10,11 +10,11 @@ | |||
"race condition" | |||
], | |||
"require": { | |||
"php": "^5.6 || ^7.0" | |||
"php": "^7.1" |
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.
Would it make sense to drop PHP 7.1 support as well? It's EOL very soon.
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.
I wasn't sure about it, as many libraries are still on PHP 7.1, and I am not really using here any PHP 7.2 features. I've updated it only because PHP 7.1 is almost EOL and I am going to still support version 1.0.
[v2.0] Drop support of PHP 5.6, 7.0 and 7.1
Update all code base to support PHP 7.2+ only.
Add strict type declaration.
Add typehints for all parameters and return types.
ExceptionInterface
now extendsThrowable
.All exceptions are not instantiable and theirs method as marked as
@internal
.I am planning release it as version 2.0.0 but still v1.0 will be maintained.
/cc @Ocramius @morozov