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

#760 - exclude http methods from processing to solve 'POST not working' issue #762

Merged
merged 1 commit into from
Oct 24, 2021

Conversation

baltun
Copy link
Contributor

@baltun baltun commented Nov 17, 2020

#760 - exclude http methods from processing to solve 'POST not working' issue

@baltun
Copy link
Contributor Author

baltun commented Dec 7, 2020

@iwasherefirst2 , please make a review and approve or give your feedback, I can explain (defend) to you my changes or can correct code or readme if needed

@iwasherefirst2
Copy link
Collaborator

I like this change, because redirecting a post request is confusing and noone wants that. This will prevent POST is not working and MethodNotAllowedHttpException issues.

Only remark from me is that I still think its good practice to localize all action routes. Imagine you have a post request to change your password. This will trigger an email an the email text depends on your locale. If you have no locale in the url, the default locale will be taken.

Because of that, I think you on the save side if you always just localize your action routes. And that is the downside of this MR, if you forget to localize your post/put/delete route, you won't be noticed by one of the above issues. Thus for me, I would probably keep httpMethodsIgnored empty in my config.

I think the only thing we may discuss here, what we agree on as the default value for httpMethodsIgnored. Otherwise, I approve this MR.

@baltun
Copy link
Contributor Author

baltun commented Dec 14, 2020

Imagine you have a post request to change your password. This will trigger an email an the email text depends on your locale.
I think it will be two different requests - one is GET to show form that need to be localized and second - POST to post data entered in form. So for form which is GET localization will works and for data which is POST redirect will not be applied and that is good
But if you still think that default value should be empty, I'll change it to empty

@iwasherefirst2
Copy link
Collaborator

Imagine you have a post request to change your password. This will trigger an email an the email text depends on your locale.
I think it will be two different requests - one is GET to show form that need to be localized and second - POST to post data entered in form

I was thinking of the default Laravel password reset function. You start with a GET request where you only enter your email. From there, you have a post request to the PasswordForgetController. This triggers an email.

Now in English you want your Email to be like this:

Hi X, did you forgot your password? If so please click on this link....

However a German user would expect:

Hi X, hast du dein Passwort vergessen? Falls dass so ist, klicke bitte auf diese Link...

If you don't localize your post route for the PasswordForgetController, this email won't be localized.

@baltun
Copy link
Contributor Author

baltun commented Dec 14, 2020

Oh, I see
But in what version of framework this route is in POST? in my not very fresh versions routes to reset password are in GET
Anyway, maybe so it would be better to make another config parameter like alwaysRedirectRoutes and add these framework routes to this parameter by default? To have POST (and other non GET) requests ignored except these manually added system (and maybe some else) not very good but used routes?

@quentingosset
Copy link

quentingosset commented Jan 31, 2021

This issue is not yet deployed on packagist ? @iwasherefirst2 @mcamara

@mcamara mcamara merged commit c71bb11 into mcamara:master Oct 24, 2021
@mcamara
Copy link
Owner

mcamara commented Oct 24, 2021

I've merged it and will create a new version, thanks for your help!

@mcamara mcamara mentioned this pull request Oct 24, 2021
Nuranto pushed a commit to Nuranto/laravel-localization that referenced this pull request Jan 14, 2022
… working' issue (mcamara#762)

Co-authored-by: Ilya Kolesnikov <ivkol@mail.ru>
(cherry picked from commit c71bb11)
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.

5 participants