-
-
Notifications
You must be signed in to change notification settings - Fork 188
Add return types for v3 #77
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
+22
−22
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ | |
| }, | ||
| "extra": { | ||
| "branch-alias": { | ||
| "dev-master": "2.0.x-dev" | ||
| "dev-master": "3.x-dev" | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hi @Crell,
It would be way more useful to apply basic types and return types in let's say v2 to cover our needs in php 7.4.
As you mention Stringable interface is introduce in php 8, so we with v7.4 are out of luck to use types where we could:
If you can publish v2 for php7.x without Stringable it would be awesome,
and keep v3 for php8.
Thanks!
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.
A child class is allowed to widen argument types (on PHP 7.2+, you can remove it, and on PHP 7.4+ you have full variance rules). So you can implement your method as
public function emergency($message, array $context = []): voidand having your implementation compatible with interfaces v1, v2 and v3 on PHP 7.2+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 did similar thing. But isn't the goal of this to abstract/extract/unify implementations so we don't have to do it one by one?
The version selector could do that for us, as said if you publish v2.x as PHP7.4 compatible, just without Stringable.
But anyway, thanks for the response.
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.
@vukanac implementations are either expected to implement exactly the PSR interface of a single supported version of psr/log or to be nice to their user regarding dependency conflicts by relying on PHP variance rules (available in PHP 7.2+) to write an implementation compatible with multiple versions of psr/log. There is no reason for psr/log to publish another version of the interface supporting PHP 7.x (which cannot be 2.x as there is already a published 2.0 signature and changing it would be a BC break). If you want to keep compatibility with PHP 7.4, keep support for psr/log 1