Skip to content

Adding type-hints and extending Throwable for exception interfaces #20

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.

Sign up for GitHub

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
weierophinney merged 1 commit into from
Mar 5, 2021
+5 −5
Merged

Adding type-hints and extending Throwable for exception interfaces #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
}
],
"require": {
"php": ">=5.3.0"
"php": ">=7.2.0"
},
"autoload": {
"psr-4": {
Expand All @@ -21,7 +21,7 @@
},
"extra": {
"branch-alias": {
"dev-master": "1.0.x-dev"
"dev-master": "1.1.x-dev"
Copy link

@derrabus derrabus Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"dev-master": "1.1.x-dev"
"dev-master": "2.0.x-dev"

Following this blog article, this change would mean a bump to 2.0.

Copy link
Member

@Jean85 Jean85 Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't. This PR doesn't add return types so, by requiring PHP 7.2, it's allowing a smooth upgrade path and a minor version is ok. #28 is instead adding return types and hence requiring a major bump.

Vote is already in progress for the approval of these two new versions, and it looks like it's passing: https://groups.google.com/g/php-fig/c/niYwPGatoz4

Copy link

@derrabus derrabus Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR doesn't add return types so, by requiring PHP 7.2, it's allowing a smooth upgrade path and a minor version is ok.

I'm not arguing with that. It's just not what has been announced in the blog article I've linked. If your processed has changed since then, that article should probably be updated.

Copy link
Member

@Jean85 Jean85 Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A newer blog post was published the month after: https://www.php-fig.org/blog/2019/12/new-bylaw-and-next-election-cycle/
And it's referring to the approval of a PHP-FIG bylaw that these PR are following: https://www.php-fig.org/bylaws/psr-evolution/

}
}
}
2 changes: 1 addition & 1 deletion src/ContainerExceptionInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@
/**
* Base interface representing a generic exception in a container.
*/
interface ContainerExceptionInterface
interface ContainerExceptionInterface extends \Throwable
Copy link

@mynameisbogdan mynameisbogdan Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks a lot of symfony installations.

Copy link
Contributor

@Ocramius Ocramius Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fixed since PHP 7.4.0+

https://3v4l.org/vUsLI

Copy link

@mynameisbogdan mynameisbogdan Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct, but 7.3 it's not yet EOL.

And thank you for the link, I wasn't aware of that change in 7.4.

As a BC change I wonder why it wasn't shipped with 2.0.0.

Copy link
Member

@Jean85 Jean85 Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, we missed this; it's a language bug that we weren't expecting. We will probably reintroduce the change bumping the PHP requirement to 7.4, see #32

{
}
4 changes: 2 additions & 2 deletions src/ContainerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface ContainerInterface
*
* @return mixed Entry.
*/
public function get($id);
public function get(string $id);

/**
* Returns true if the container can return an entry for the given identifier.
Expand All @@ -33,5 +33,5 @@ public function get($id);
*
* @return bool
*/
public function has($id);
public function has(string $id);
Copy link

@odan odan Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add :bool as return type? Example:

public function has(string $id): bool;

Copy link
Contributor

@gnumoksha gnumoksha Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This question is addressed in the last paragraph :)

Copy link

@odan odan Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks :-) I think this is handled on the basis of the new PHP version 7.2 but ok.

}