-
Notifications
You must be signed in to change notification settings - Fork 53
Upgrade to PHP 7.1+ #18
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
Conversation
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 is my evaluation, as per adherence to the evolution bylaw. Changes overall are good, but they would need to be played out in two steps, 1.1 and 2.0, to ease the migration path for implementing libraries and consumers.
@@ -2,3 +2,4 @@ build | |||
composer.lock | |||
docs | |||
vendor | |||
.idea |
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 is an opinionated ignore, I would prefer not to have this.
@@ -10,7 +10,7 @@ | |||
} | |||
], | |||
"require": { | |||
"php": ">=5.3.0" | |||
"php": ">=7.1.0" |
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.
Why not ^7.0
? We cannot use void
anyhow without releasing it as a new BC major...
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.
Oh, I see that we would need it for iterable
. IMHO it's a good trade off then, since a great number of libs jumped directly from 5 to ^7.1
due to void
.
@@ -19,7 +19,7 @@ | |||
}, | |||
"extra": { | |||
"branch-alias": { | |||
"dev-master": "1.0.x-dev" | |||
"dev-master": "2.0.x-dev" |
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.
As said before, I would prefer stepping through a 1.1 before, as per our PSR evolution bylaw
* | ||
* @throws \Psr\SimpleCache\InvalidArgumentException | ||
* MUST be thrown if the $key string is not a legal value. |
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.
There are additional evaluations to be completed here, we cannot drop this. The spec says:
Key - A string of at least one character that uniquely identifies a cached item. Implementing libraries MUST support keys consisting of the characters A-Z, a-z, 0-9, _, and . in any order in UTF-8 encoding and a length of up to 64 characters. Implementing libraries MAY support additional characters and encodings or longer lengths, but MUST support at least that minimum. Libraries are responsible for their own escaping of key strings as appropriate, but MUST be able to return the original unmodified key string. The following characters are reserved for future extensions and MUST NOT be supported by implementing libraries: {}()/@:
*/ | ||
public function set($key, $value, $ttl = null); | ||
public function set(string $key, $value, $ttl = null):bool; |
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.
We cannot add a return type without BC and a new major.
|
||
/** | ||
* Wipes clean the entire cache's keys. | ||
* | ||
* @return bool True on success and false on failure. | ||
*/ | ||
public function clear(); | ||
public function clear():bool; |
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.
Return type is a BC here too.
*/ | ||
public function getMultiple($keys, $default = null); | ||
public function getMultiple(iterable $keys, $default = null):iterable; |
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.
Return type is a BC here too.
*/ | ||
public function setMultiple($values, $ttl = null); | ||
public function setMultiple(iterable $values, $ttl = null):bool; |
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.
Return type is a BC here too.
*/ | ||
public function deleteMultiple($keys); | ||
public function deleteMultiple(iterable $keys):bool; |
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.
Return type is a BC here too.
*/ | ||
public function has($key); | ||
public function has(string $key):bool; |
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.
Return type is a BC here too.
Still, as per our bylaws those changes would need to ho through a CC vote. |
Hey, thank you for picking this up! I'm very aware that most of these changes are breaking and would require a vote. I made this PR originally to signal the necessity for a change that is far too long overdue for several PSRs now. I'm pretty sure that the FIG is also aware, however, I am - probably like many others - disappointed by the slow pace of picking up new major PHP features. 7.1 was released in 2016 and is already EOL. Anyway, i guess it's easier for y'all to create new PRs to support version stepping as you intend, rather than me adding more changes to this PR. |
Another PHP 7 upgrade wish! ✨