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

[11.x] Fix: Ensure generated policies return boolean values #53630

Closed

Conversation

Aluisio-Pires
Copy link

Summary

This PR resolves an issue where the methods in policies generated via commands such as make:model ModelName --policy returned a bool type but lacked implementations, causing PhpStan to report type errors. The problem stems from the empty method bodies in the default policy stubs.

Changes Made

  • Updated the policy stub to include default true return values for all methods:
    • viewAny
    • view
    • create
    • update
    • delete
    • restore
    • forceDelete

This ensures the generated methods comply with their defined return types and eliminates PhpStan type-checking errors.

How to Test

  1. Generate a new model with a policy using php artisan make:model Example --policy.
  2. Inspect the generated policy methods for default true return values.
  3. Confirm that running static analysis (e.g., PhpStan) no longer raises type errors for the generated policy.

Impact

This change improves developer experience by making generated policies immediately usable without requiring manual fixes to satisfy static analysis.

Let me know if further details are needed!

@Jubeki
Copy link
Contributor

Jubeki commented Nov 22, 2024

This would be a security issue. If this were to be changed, it should be deny by default.

@taylorotwell
Copy link
Member

See above.

@Aluisio-Pires
Copy link
Author

This would be a security issue. If this were to be changed, it should be deny by default.

Really good point. I was avoiding the same FormRequests behavior were we are forced to change the authorize return, but I see the issue. I will reopen de PR changing to false.

@Aluisio-Pires
Copy link
Author

See above.

@taylorotwell I've created a new PR following the recommendations.

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

Successfully merging this pull request may close these issues.

3 participants