renderdata() enhancement #328

Closed
jochemd opened this Issue May 11, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@jochemd
Contributor

jochemd commented May 11, 2015

In 3.1 devel a new renderData type "rawjson" is supported. I can think of at least 3 more types I would want:

Compared to the existing renderData they all have the problem they need more arguments since you need to pass in a mime-type as well. So might it be an idea to somehow make this extensible?

Discussion at https://groups.google.com/forum/#!topic/framework-one/aY5Ct68IbdE

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield May 15, 2015

Member

The current assumption is that "rendering" produces something that can be passed to writeOutput() to be written into the output stream. I'm not sure how these would sit with that machinery.

One way I can think of is to allow for renderData()s type argument to be a function, that takes the data and returns a struct with contentType, output, and optional writer fields. The logic of renderDataWithContentType() would change so that if type was a UDF/closure, it would call the type function on the data, set the content type based on the result and return the whole struct instead of just out (the normally transformed data).

onRequest() would then change to check whether out was a string or a struct and, if a string, continue to call writeOutput() on it, else it would assume the struct contained output and, if writer exists then call writer on the output, else call writeOutput on it.

This would also have the side effect of allowing an overridden onMissingView() function to return a struct containing output and writer so it could do something other than just outputting an HTML string. Hmm, that would speak toward setting the content type in onRequest() to allow onMissingView() to override the content type easily.

Thoughts? Is that too "clever"?

Member

seancorfield commented May 15, 2015

The current assumption is that "rendering" produces something that can be passed to writeOutput() to be written into the output stream. I'm not sure how these would sit with that machinery.

One way I can think of is to allow for renderData()s type argument to be a function, that takes the data and returns a struct with contentType, output, and optional writer fields. The logic of renderDataWithContentType() would change so that if type was a UDF/closure, it would call the type function on the data, set the content type based on the result and return the whole struct instead of just out (the normally transformed data).

onRequest() would then change to check whether out was a string or a struct and, if a string, continue to call writeOutput() on it, else it would assume the struct contained output and, if writer exists then call writer on the output, else call writeOutput on it.

This would also have the side effect of allowing an overridden onMissingView() function to return a struct containing output and writer so it could do something other than just outputting an HTML string. Hmm, that would speak toward setting the content type in onRequest() to allow onMissingView() to override the content type easily.

Thoughts? Is that too "clever"?

@jochemd

This comment has been minimized.

Show comment
Hide comment
@jochemd

jochemd May 15, 2015

Contributor

If users have to write a closure, then what is the benefit over just uising cfcontent + cfabort directly? Also (and this is a somewhat new consideration), a user would either use X-Sendfile if he runs Apache, or X-Accell if he uses Nginx or cfcontent if neither is supported. The choice between them should be driven from the FW/1 config and not be put in the closure code.

So I am more thinking along the lines of just adding a third argument to renderData for the MIME type and then putting logic in to check the FW/1 (environment) configuration whether X-Sendfile or X-Accell are enabled. If one of them is, use it, else fall back to cfcontent.
What I see as an open issue with this design is that I would like to make it possible to supply either a path or a bytearray as the resultData parameter and let renderData figure out the rest. But other than doing an isString() plus a fileExists() I don't see a way to figure out whether the resultData is a path or a bytearray, and I don't like the performance overhead of doing so. But even without solving that adding both types "binary" for a bytearray and "file" for a path seems like a better solution to me than using a closure.

Contributor

jochemd commented May 15, 2015

If users have to write a closure, then what is the benefit over just uising cfcontent + cfabort directly? Also (and this is a somewhat new consideration), a user would either use X-Sendfile if he runs Apache, or X-Accell if he uses Nginx or cfcontent if neither is supported. The choice between them should be driven from the FW/1 config and not be put in the closure code.

So I am more thinking along the lines of just adding a third argument to renderData for the MIME type and then putting logic in to check the FW/1 (environment) configuration whether X-Sendfile or X-Accell are enabled. If one of them is, use it, else fall back to cfcontent.
What I see as an open issue with this design is that I would like to make it possible to supply either a path or a bytearray as the resultData parameter and let renderData figure out the rest. But other than doing an isString() plus a fileExists() I don't see a way to figure out whether the resultData is a path or a bytearray, and I don't like the performance overhead of doing so. But even without solving that adding both types "binary" for a bytearray and "file" for a path seems like a better solution to me than using a closure.

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield May 15, 2015

Member

The reason I suggested the UDF/closure is because nothing in FW/1 uses cfcontent and there are no tags in the code so I'm not sure there's a portable way to deal with that in script -- plus everyone pays for more conditional logic which I'm trying to keep to a minimum by isolating behavior into user-supplied code.

Adding additional arguments that are conditionally used (what would type be in that case?) and ad hoc configuration and conditional logic sprinkled across the framework doesn't sound like very good design to me when what's wanted here is to abstract behavior.

If there are certain, standard, file-processing behaviors, then FW/1 could provide UDFs that encapsulated those, for use as type arguments.

I'd be interested to see a PR with the code changes for what you are proposing so we'd have something concrete to review (but right now I'm not 100% sure exactly what you are proposing, especially around the send file / accell config).

Member

seancorfield commented May 15, 2015

The reason I suggested the UDF/closure is because nothing in FW/1 uses cfcontent and there are no tags in the code so I'm not sure there's a portable way to deal with that in script -- plus everyone pays for more conditional logic which I'm trying to keep to a minimum by isolating behavior into user-supplied code.

Adding additional arguments that are conditionally used (what would type be in that case?) and ad hoc configuration and conditional logic sprinkled across the framework doesn't sound like very good design to me when what's wanted here is to abstract behavior.

If there are certain, standard, file-processing behaviors, then FW/1 could provide UDFs that encapsulated those, for use as type arguments.

I'd be interested to see a PR with the code changes for what you are proposing so we'd have something concrete to review (but right now I'm not 100% sure exactly what you are proposing, especially around the send file / accell config).

@aliaspooryorik

This comment has been minimized.

Show comment
Hide comment
@aliaspooryorik

aliaspooryorik Nov 18, 2015

Contributor

I was discussing the idea of allowing the JSON serialisation to be switched out (as SerializeJSON has several known issues on ACF!). Adding a comment here as it's loosely related.

My thinking was to do a simple public SerializeAsJSON function which could be overridden by the developer if required. Something like this commit:
aliaspooryorik@cf67649

Looking through the code for 4.0 I can see that there is a move towards using a builder where you could pass in a function to handle this instead.

Contributor

aliaspooryorik commented Nov 18, 2015

I was discussing the idea of allowing the JSON serialisation to be switched out (as SerializeJSON has several known issues on ACF!). Adding a comment here as it's loosely related.

My thinking was to do a simple public SerializeAsJSON function which could be overridden by the developer if required. Something like this commit:
aliaspooryorik@cf67649

Looking through the code for 4.0 I can see that there is a move towards using a builder where you could pass in a function to handle this instead.

@seancorfield seancorfield added the ready label Nov 28, 2015

@seancorfield seancorfield self-assigned this Nov 28, 2015

@seancorfield seancorfield added this to the 4.0 milestone Nov 28, 2015

@seancorfield seancorfield added in progress and removed ready labels Nov 28, 2015

seancorfield added a commit that referenced this issue Nov 28, 2015

Allow render type to be a function #328
This allows custom renderers for built-in types by overriding the
render_{type}() function or supplying a function or closure for the
renderData() type argument.
@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Nov 28, 2015

Member

renderData()s type can now be a UDF/closure that must return a struct containing contentType and output and an optional writer function per my notes above. I have not yet updated onMissingView() to support a struct return.

Member

seancorfield commented Nov 28, 2015

renderData()s type can now be a UDF/closure that must return a struct containing contentType and output and an optional writer function per my notes above. I have not yet updated onMissingView() to support a struct return.

seancorfield added a commit that referenced this issue Nov 28, 2015

Lift contentType to response #328
This allows onMissingView() to control content type as well as renderData().
@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Nov 28, 2015

Member

onMissingView() can now return a struct with contentType, output, and an optional writer, and can affect the content type. Just documentation to update now.

Member

seancorfield commented Nov 28, 2015

onMissingView() can now return a struct with contentType, output, and an optional writer, and can affect the content type. Just documentation to update now.

seancorfield added a commit that referenced this issue Nov 28, 2015

ACF compatibility #328
Trailing comma in struct. Sigh.

seancorfield added a commit that referenced this issue Nov 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment