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.

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

Jump to bottom

[Downgrade PHP 7.0] Add Exception fallback for instanceof Throwable #1608

Merged
merged 5 commits into from
Jan 4, 2022
Merged

[Downgrade PHP 7.0] Add Exception fallback for instanceof Throwable #1608

merged 5 commits into from
Jan 4, 2022

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jan 1, 2022

No description provided.

@jtojnar jtojnar marked this pull request as draft January 1, 2022 13:35
@jtojnar jtojnar marked this pull request as ready for review January 1, 2022 14:08
@jtojnar jtojnar force-pushed the instanceof-throwable branch 2 times, most recently from 95f974e to 44c089c Compare January 1, 2022 16:42
@jtojnar jtojnar mentioned this pull request Jan 2, 2022
Merged
@jtojnar jtojnar force-pushed the instanceof-throwable branch 5 times, most recently from 4e238a3 to 617ffca Compare January 2, 2022 13:20
jtojnar
jtojnar commented Jan 2, 2022
View reviewed changes
rules/DowngradePhp70/Rector/Instanceof_/DowngradeInstanceofThrowableRector.php
}

$expectedDisjunct = $this->createFallbackCheck($var);
return $this->nodeComparator->isNodeEqual($expectedDisjunct, $disjuncts);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this extended algorithm, it will also skip $e instanceof \Exception || ($e = $e->getPrevious()) || $e instanceof Throwable, which the previous variant would correctly change to $e instanceof \Exception || ($e = $e->getPrevious()) || ($e instanceof Throwable || $e instanceof \Exception)

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 2, 2022 •
edited

Also, should I move each commit into a separate PR? The merge method you use appears to be destructive (squashes independent commits and removes commit messages).

@jtojnar jtojnar force-pushed the instanceof-throwable branch 2 times, most recently from a845807 to ad8194a Compare January 3, 2022 13:10
samsonasik
samsonasik reviewed Jan 3, 2022
View reviewed changes
packages/NodeCollector/BinaryOpConditionsCollector.php
*/
public function findConditions(BinaryOp $binaryOp, string $binaryOpClass): array
public function findConditions(Expr $binaryOp, string $binaryOpClass): array
Copy link
Member

Choose a reason for hiding this comment

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

I think BinaryOp should be kept to ensure the usage is on BinaryOp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the last commit, it makes for much nicer definitions that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Especially with b76243b.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test case and comment making it clearer.

@jtojnar
[Downgrade PHP 7.0] Add Exception fallback for instanceof Throwable
e987f2e 
PHP 7.0 introduced Throwable and changed the exception hierarchy,
adding Throwable as its new root. This means various error handlers
will now check for Throwable:

    $e instanceof Throwable

To preserve compatibility with PHP 5.6, we need to check for both
Throwable and Exception. And since the value can possibly produce side-effects,
we need to assign it to a temporary variable:

    (($throwable = $loggedMessage->getExtraObject()) instanceof Throwable || $throwable instanceof \Exception)
@jtojnar
Add BinaryOpTreeRootLocator helper
103bc17
@jtojnar
BinaryOpConditionsCollector: Make return type more specific
1c66dbb 
This is to prevent the following PHPStan error:

rules/DowngradePhp70/Rector/Instanceof_/DowngradeInstanceofThrowableRector.php:124
- '#Parameter \#2 \$offset of function array_slice expects int, int\|string given#'
@jtojnar
DowngradeInstanceofThrowableRector: Handle some isomorphic cases
149ce69 
Previously, we ensured idempotency by checking for one of the following expressions:

    (<var> = <expr>) instanceof Throwable || <var> instanceof Exception
    <var> instanceof Throwable || <var> instanceof Exception

The latter was used to detect similar manual transformation,
which would not use assignment because variable access is side-effect free.

But in that case, it makes sense to also allow the opposite order

    <var> instanceof Exception || <var> instanceof Throwable

or even extra disjuncts:

    <var> instanceof Exception || foo() || bar() || <var> instanceof Throwable
@jtojnar
BinaryOpConditionsCollector: Allow non-BinaryOp nodes
f65d464 
This allows nicer definition of some tree operations
by considering such nodes the trivial case of single operand.
@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 4, 2022

Rebased.

@TomasVotruba
Copy link
Member

Thank you 👏

@TomasVotruba TomasVotruba merged commit 0a0e0fd into rectorphp:main Jan 4, 2022
@jtojnar jtojnar deleted the instanceof-throwable branch January 4, 2022 10:12
@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 4, 2022

Looks like the merge destroyed the commit messages again 😿

@TomasVotruba
Copy link
Member

What do you mean?
We use squash merge here, to keep PR rich for mutual references and main branch short.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 4, 2022

Well then the commit messages are lost from git history (no way to get them e.g. with git blame).

@TomasVotruba
Copy link
Member

Yes, that's expected trade off.
The GitHub handles it very well.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 4, 2022 •
edited

Keeping main branch short does not really sound like a benefit (by this metric, you could just keep squashing everything into the single initial commit). Well-crafted commits in a PR should already be short and independent and grouping them into PRs is only done because Git forge UX is bad for reviewing patch sets.

Commits like the ones in this PR should IMO be preserved in the git history so that developer can run git blame – even GitHub UI runs blame on the main branch, not PRs. And GitHub is far inferior to well configured development environment. And that is without factoring in the vendor lock-in – I still remember Gitorious, and BitBucket, and Sourceforge.

If you want nice and independent commits in the history, require it in review (interactive rebase is developer’s best friend). If you want a linear history, rebase before merging. If you want to associate a pull-request with the commits, GitHub already shows that association on the commit page.

And it might also be useful to look into merge automation. For GitLab, Marge bot works quite well for GNOME, it also adds Part-of: <link> to each commit message on merge (example) but am not familiar with any alternatives for GitHub.

@TomasVotruba
Copy link
Member

Thanks for taking time to write the reply.

It seems important to you and I don't have any strong opinion about this, so let's give your suggestion try 👍

image

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 4, 2022

Thanks. While the GitHub merges will produce messy history with “tramlines” (lines linking to parents in commit graph resembling a public transport lines map), it is indeed better than no history at all. Or you might prefer rebase merging if you want a linear history. Unfortunately, GitHub does not offer anything better without running custom bots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsonasik samsonasik samsonasik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jtojnar @TomasVotruba @samsonasik