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

Fix forgotten exception throws #10489

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Conversation

janedbal
Copy link
Contributor

@janedbal janedbal commented Feb 3, 2023

I believe those exceptions are meant to be thrown, right? Otherwise, this code is useles.

Detected by https://github.com/shipmonk-rnd/phpstan-rules#forbidunusedexception

greg0ire
greg0ire previously approved these changes Feb 3, 2023
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Oh wow good catch! I'm pretty careless sometimes 🤦

@greg0ire
Copy link
Member

greg0ire commented Feb 3, 2023

Argh it breaks the test suite… somehow we are relying on this feature, which does not work with Persistence 3, and somehow that did not break the tests before 😓

I should have changed them in #9592

@greg0ire
Copy link
Member

greg0ire commented Feb 3, 2023

I'm looking into it.

@greg0ire greg0ire added this to the 2.14.2 milestone Feb 3, 2023
@janedbal
Copy link
Contributor Author

janedbal commented Feb 3, 2023

@greg0ire Do you want me to send PR with this rule enabled to make sure this error wont happen again? It is pretty common error actually :)

@derrabus derrabus added the Bug label Feb 3, 2023
@derrabus derrabus merged commit 8debb92 into doctrine:2.14.x Feb 3, 2023
@greg0ire
Copy link
Member

greg0ire commented Feb 4, 2023

@janedbal I think adding a dependency for one rule isn't a great idea. Maybe there are more rules worth implementing?

@janedbal
Copy link
Contributor Author

janedbal commented Feb 6, 2023

There might be some, but the package is aiming to more strict codebases than this one is, so many of them could not be enabled. Also, it is tested only on PHP 7.4+. So probably not worth the effort.

@PhilETaylor
Copy link

PhilETaylor commented Apr 13, 2023

this created this b/c break in a minor release :( #10621

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

Successfully merging this pull request may close these issues.

4 participants