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

Make sure baked controllers follow CakePHP codding standard #1197

Closed
wants to merge 1 commit into from

Conversation

borivojevic
Copy link

Make sure baked controllers follow CakePHP codding standard

@borivojevic
Copy link
Author

Throwing Method Not Allowed exception in delete method was initially introduced in e380b76 but seem to be removed over time.

I noticed that PHP code sniffer throws errors like Expected 1 @throws tag(s) in function comment; 2 found for newly baked controllers.

This commit solves the inconsistency with @throws declarations in delete method's docblock and ultimately it makes sense not to expose delete methods to be accessible via GET requests.

@ceeram
Copy link
Contributor

ceeram commented Mar 26, 2013

You can not throw that without defining which methods are allowed.
http specs require to list the allowed methods, we had discussed adding this to bake, but had decided not to.

Exceptions now have the a method to define headers to include the allowed methods:
d4986b5

@dereuromark
Copy link
Member

Also, if you want to include them in your baked controllers, you just have to create your own custom bake templates with that snippet included - what I do for all my (admin) delete actions.

@borivojevic
Copy link
Author

@ceeram

Oh I understand. Perhaps just removing unnecesary @throws tag makes more sense in order to make baked controllers clean from CakePHP codding standard perspective.

@dereuromark

That is the exact thing I was doing. Thanks for sharing the trick :)

@ADmad
Copy link
Member

ADmad commented Mar 26, 2013

@borivojevic I removed the extraneous @throws in 9d367e1

@ADmad ADmad closed this Mar 26, 2013
@borivojevic
Copy link
Author

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.

None yet

4 participants