Skip to content
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

Add return types for v3 #77

Merged
merged 1 commit into from
Jul 14, 2021
Merged

Add return types for v3 #77

merged 1 commit into from
Jul 14, 2021

Conversation

Crell
Copy link
Collaborator

@Crell Crell commented Jun 4, 2021

The return types are all void, so this is a pretty boring addition. This necessarily includes the v2 branch, which should be reviewed in its own PR #76.

See the one extra commit for the specific changes in this PR.

@Crell Crell force-pushed the v3 branch 2 times, most recently from 0caaede to 3446186 Compare June 9, 2021 17:32
@Crell Crell mentioned this pull request Jul 14, 2021
@@ -27,7 +27,7 @@ interface LoggerInterface
*
* @return void
*/
public function emergency(string|\Stringable $message, array $context = []);
public function emergency(string|\Stringable $message, array $context = []): void;
Copy link

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:

  • return type
  • ?LoggerInterface $logger = null; in LoggerAwareTrait.

If you can publish v2 for php7.x without Stringable it would be awesome,
and keep v3 for php8.

Thanks!

Copy link
Contributor

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 = []): void and having your implementation compatible with interfaces v1, v2 and v3 on PHP 7.2+

Copy link

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.

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants