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

[Downgrade PHP 7.0] Move Throwable out of type hints #1602

Merged
merged 1 commit into from
Jan 1, 2022

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 31, 2021

It is only available on PHP 7.0+:
https://www.php.net/manual/en/class.throwable.php

@jtojnar
Copy link
Contributor Author

jtojnar commented Dec 31, 2021

This is a first part of work to be able to use latest Tracy on PHP 5.6. I need it for selfoss RSS reader, which still targets PHP 5.6 through 8.1. I also want to migrate try { … } catch (Throwable $e) { … } to try { … } catch (Throwable $e) { … } catch (Exception $e) { … } and … instanceof \Throwable to … instanceof \Throwable || … instanceof \Exception.

@TomasVotruba
Copy link
Member

Hi, thanks for the new rule.

I wonder if RenameClassRector might help with this? E.g. Throwable to Exception

@jtojnar
Copy link
Contributor Author

jtojnar commented Dec 31, 2021

The issue is that it is not a straightforward renaming. Especially when we want the resulting code to support both PHP 5 and PHP 7. (I am not even sure if we should make it part of downgrade set.)

Though maybe we could have something like RenameClassRector that would change Throwable into Throwable|Exception and then have other rules resolve the other cases.

@TomasVotruba
Copy link
Member

I see. In that way custom rule might be a way to go 👍

@jtojnar jtojnar force-pushed the throwable branch 2 times, most recently from eab6b55 to 2ac627c Compare January 1, 2022 00:43
Comment on lines +21 to +22
* @param \Throwable|null $anything
* @return \Throwable|null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not preserve formatting (changes ?\Throwable to \Throwable|null. But that is probably fine.

Copy link
Member

Choose a reason for hiding this comment

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

that's actually fine https://phpstan.org/r/5c17f3ca-4b93-44e4-9597-b21bb9b784c5 , improvement maybe in separate PR.

public function getRuleDefinition(): RuleDefinition
{
return new RuleDefinition(
'Remove `Throwable` return type, add a `@return Throwable` tag instead',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading the docs again, we need to mention that it does params too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it looks like the examples are not tested since I based this on DowngradeSelfTypeDeclarationRector and it does not have the documentation comment either.

@samsonasik samsonasik merged commit 7d927c9 into rectorphp:main Jan 1, 2022
@samsonasik
Copy link
Member

Thank you @jtojnar

@jtojnar jtojnar deleted the throwable branch January 1, 2022 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants