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

[12.x]Fix PHPDoc return types #54999

Open
wants to merge 2 commits into
base: 12.x
Choose a base branch
from

Conversation

mohammadrasoulasghari
Copy link
Contributor

@mohammadrasoulasghari mohammadrasoulasghari commented Mar 12, 2025

In this PR, I've made three improvements to enhance code clarity and type accuracy without changing any functionality.🧹✨

1. Updated forceDestroy() return type to int

The forceDestroy() method directly returns the result of destroy(), which returns an integer (the count of deleted records). Updated the PHPDoc to accurately reflect this relationship for better type consistency.

2. Aligned resolveConnection() return type with interface definition

Updated the return type from Connection to ConnectionInterface to match the actual return type from the underlying resolver's connection() method. This ensures type consistency and improves IDE support when working with connection objects.

I noticed these issues while refactoring the Model class and thought it would be beneficial to update them for better code quality and developer experience.🔍👨‍💻

@mohammadrasoulasghari mohammadrasoulasghari force-pushed the fix/types-and-refactor-Model branch from 3107fcb to 792f081 Compare March 12, 2025 21:02
@rodrigopedra
Copy link
Contributor

  1. Simplified conditional logic using logical OR operator

Replaced a ternary conditional expression with a more readable logical OR operator that maintains identical behavior while making the code more concise and easier to understand.

I guess the "easier to understand" argument is very subjective here.

I find the previous code easier to understand, for these reasons:

  • It is more declarative, as it doesn't rely on knowledge of how a boolean operator short-circuits
  • It doesn't rely on an inverted logic by using the ! (not) operator
  • Thus, for newer devs, it doesn't raise questions on why the condition couldn't be flipped

I am all in for the PHP docs fixes, but I feel this other change should be sent as a separate PR.

The forceDestroy method directly returns the result of the destroy method,
@mohammadrasoulasghari mohammadrasoulasghari force-pushed the fix/types-and-refactor-Model branch from 792f081 to 3638ce8 Compare March 13, 2025 00:16
…face

Updated the PHPDoc return type from Connection to ConnectionInterface
to match the actual return type from the underlying resolver's connection method.
This provides better type consistency across the codebase.
@mohammadrasoulasghari mohammadrasoulasghari changed the title [12.x]Fix PHPDoc return types and refactor conditional expression [12.x]Fix PHPDoc return types Mar 13, 2025
@mohammadrasoulasghari
Copy link
Contributor Author

mohammadrasoulasghari commented Mar 13, 2025

  1. Simplified conditional logic using logical OR operator

Replaced a ternary conditional expression with a more readable logical OR operator that maintains identical behavior while making the code more concise and easier to understand.

I guess the "easier to understand" argument is very subjective here.

I find the previous code easier to understand, for these reasons:

  • It is more declarative, as it doesn't rely on knowledge of how a boolean operator short-circuits
  • It doesn't rely on an inverted logic by using the ! (not) operator
  • Thus, for newer devs, it doesn't raise questions on why the condition couldn't be flipped

I am all in for the PHP docs fixes, but I feel this other change should be sent as a separate PR.

Thanks
You’re right, I’ve reverted the change back to the original ternary expression.

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.

2 participants