Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Add get decode json data on params controller plugin #5897

Closed
wants to merge 4 commits into from
Closed

Add get decode json data on params controller plugin #5897

wants to merge 4 commits into from

Conversation

lies
Copy link

@lies lies commented Mar 3, 2014

No description provided.

@Ocramius Ocramius added this to the 2.3.0 milestone Mar 3, 2014
*/
public function fromJsonRawBody($param = null, $default = null)
{
if (empty($this->jsonDecoded)) {
Copy link
Member

Choose a reason for hiding this comment

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

@EvanDotPro you suggested caching the decoded json - is it such a big deal here?

I see more problems than anything coming from this, since the plugin is now unusable over multiple requests and/or internal forwards.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, after re-reading the method body, caching the decoded json is an overkill here. I'd just suggest getting rid of this.

Sorry about that, @lies

@Ocramius
Copy link
Member

Ocramius commented Mar 3, 2014

@lies for further changes, just push new commits to this branch - that will automatically update the pull request

@weierophinney
Copy link
Member

I'm going to say "no" to this PR. The reason is that we really shouldn't be pushing this sort of functionality directly into the controllers; I'd actually prefer to be removing functionality from the base controllers at this point. Functionality like this should be via controller plugins. We've actually implemented this exact functionality already via zfcampus/zf-content-negotiation, in the bodyParams plugin. This allows you to do the following:

$params = $this->bodyParams();

or, to retrieve a single parameter:

$param = $this->bodyParam('name', 'defaultValue');

An approach like this keeps the ZF2 MVC lighter, makes the functionality opt-in, and, in this case, provides more possibilities for extension to support a larger number of mediatypes.

@lies
Copy link
Author

lies commented Mar 4, 2014

:)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants