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

minor changes #54161

Closed
wants to merge 2 commits into from
Closed

minor changes #54161

wants to merge 2 commits into from

Conversation

isaeken
Copy link
Contributor

@isaeken isaeken commented Jan 11, 2025

This pull request includes minor changes to improve code clarity and functionality in the src/Illuminate/Auth/Access/Gate.php and src/Illuminate/Foundation/Http/FormRequest.php files.

Code clarity and functionality improvements:

This ensures better IDE support and improves code readability by explicitly declaring the exception type. No functional changes are introduced, preserving existing behavior.
Previously, the method implicitly returned null, which could lead to confusion or inconsistencies. Explicitly returning null ensures clarity and aligns with expected behavior.
@shaedrich
Copy link
Contributor

shaedrich commented Jan 11, 2025

request to branches that are currently supported: laravel.com/docs/releases#support-policy

If you are unsure which branch your pull request should be sent to, please read: laravel.com/docs/contributions#which-branch

Pull requests without a descriptive title, thorough description, or tests will be closed.

In addition, please describe the benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

It looks like you accidentally included that into your PR description by removing a HTML comment 🤔

@isaeken
Copy link
Contributor Author

isaeken commented Jan 11, 2025

request to branches that are currently supported: laravel.com/docs/releases#support-policy
If you are unsure which branch your pull request should be sent to, please read: laravel.com/docs/contributions#which-branch
Pull requests without a descriptive title, thorough description, or tests will be closed.
In addition, please describe the benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

It looks like you accidentally included that into your PR description by removing a HTML comment 🤔

Yes i made a mistake it has been corrected.

@@ -165,6 +165,7 @@ protected function failedValidation(Validator $validator)
{
$exception = $validator->getException();

/** @var \Illuminate\Validation\ValidationException $exception */
Copy link
Contributor

@rodrigopedra rodrigopedra Jan 12, 2025

Choose a reason for hiding this comment

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

How is $exception is not a string?

The return type of Illuminate\Validation\Validator@getException() is class-string<\Illuminate\Validation\ValidationException>, so $exception is a string.

/**
* Get the exception to throw upon failed validation.
*
* @return class-string<\Illuminate\Validation\ValidationException>
*/
public function getException()
{
return $this->exception;
}

Furthermore, I don't think the new operator can be used with anything other than a class name, or that evaluates to a string representing a class name.

EDIT: clarification about new operator usage.

@@ -572,6 +572,8 @@ protected function callBeforeCallbacks($user, $ability, array $arguments)
return $result;
}
}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I also prefer that all null returns would be explicit, there are too many places in Laravel that rely on null to be implicitly returned.

I'd really like to see this particular change merged, and maybe done for all other places to make everything more explicit, but I wouldn't hold high hopes on that one.

@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

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.

4 participants