Skip to content

Conversation

@bobbybouwmann
Copy link
Contributor

See #282

Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

Nice job. Can you also set a maxlength attribute on the input field of the thread subject? Both in the create and update screen.

{
return [
'subject' => 'required|'.DoesNotContainUrlRule::NAME.'|'.SpamRule::NAME,
'subject' => 'required|max:'.self::SUBJECT_SIZE.'|'.DoesNotContainUrlRule::NAME.'|'.SpamRule::NAME,
Copy link
Member

Choose a reason for hiding this comment

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

You can replace the constant with 60 in the rule itself here.


namespace Tests\Feature;

use App\Http\Requests\ThreadRequest;
Copy link
Member

Choose a reason for hiding this comment

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

You can move the constant from the request to the test case. The reason why you should do this is because your test serves as a sort of contract which means you should try to be as explicit as possible for defining values instead of relying on imported constants. Say someone changes the value in the validator for some reason, you're never going to notice because it's inherited in the test. It's important for this test that the value is 60.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I will make some changes ;)

@driesvints driesvints merged commit 6a07b28 into laravelio:master Oct 21, 2017
@driesvints
Copy link
Member

Thanks Bobby!

piotr-fraczek added a commit to piotr-fraczek/portal that referenced this pull request Oct 21, 2017
Set the max length of the subject to 60 (laravelio#324)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants