3.1 - Jsonview auto serialize #6502

ADmad · 2015-05-06T21:27:00Z

ceeram · 2015-05-06T22:59:49Z

src/View/JsonView.php

- * views to provide layout-like functionality.
+ * If you need the regular view template processing you can set `_serialize` to
+ * false which will turn off automatic serialization and instead view template
+ * and layotu will be used.


nitpick typo in layout

Not a nitpick, this ends up in API docs :)

ceeram · 2015-05-06T23:00:06Z

lgtm

ceeram · 2015-05-06T23:00:43Z

Shouldnt XmlView behave similar?

markstory · 2015-05-07T01:30:13Z

XmlView should behave in a similar way. Having them be different will be not good. Also this change could be interpreted as an API breaking change. If an app was relying on using view files, it would now default to serializing which could be unexpected. What was wrong with the serialize => true approach?

ADmad · 2015-05-07T05:55:59Z

What was wrong with the serialize => true approach?

It would still require adding $this->set('_serialize', true) in all actions which isn't DRY.

Doing so will serialize all available view variables.

Set '_serialize' to false to turn off serialization.

markstory · 2015-05-07T14:58:02Z

src/View/JsonView.php

- * When the view is rendered, the `$posts` view variable will be serialized
- * into JSON.
+ * By default all view variables will be serialized to JSON. So for above example
+ * the `$posts` view variable will be serialized into JSON.


Is this still true?

josegonzalez · 2015-05-07T15:07:02Z

Seems like an API break, and perhaps a serious one for certain users. As such, I'm 👎 .

Do we want to move to breaking the API in minor releases?

patrickconroy · 2015-05-07T15:45:30Z

Would a controller property work? So, setting serialize in your AppController to true would auto-serialize the json view? And it'd default to false?

Right now all my json stuff has that serialize view property which is cumbersome.

dereuromark · 2015-05-07T16:12:10Z

@patrickconroy You can set $this->set('_serialize', true) in your AppController. This way it would not hurt if some controllers don't use it, but it would apply to all by default that use JsonView.
That is pretty DRY in my book - and it is fully BC.

So that's what we should do here IMO.
I think less magic and explicitly setting it to true is reasonable.

ADmad · 2015-05-07T16:42:27Z

I'll switch back to not serializing by default to avoid BC issues. Instead can document the fact that one can set $this->set('_serialize', true) in AppController::beforeRender() to get serializing as default behavior.

markstory · 2015-05-07T17:05:45Z

Instead can document the fact that one can set $this->set('_serialize', true) in AppController::beforeRender() to get serializing as default behavior.

This sounds like a much safer plan. We could even make this part of the AppController in the project skeleton.

ADmad · 2015-05-07T17:07:10Z

Done.

dereuromark · 2015-05-07T17:08:23Z

This would probably have fit into 3.0 now as well. But with 3.1 it can be communicated better.
Also we should make XmlView allow true here.

ADmad · 2015-05-07T17:15:38Z

This would probably have fit into 3.0 now as well.

No it won't as this change causes this test to fail.

dereuromark · 2015-05-07T17:18:55Z

Doesn't make much sense, as you said :) But all right, 3.1 is fine.

It contains common code used by view which return serialized data.

ADmad · 2015-05-08T07:31:32Z

Implemented support for _serialize true for XmlView too.
Pulled out duplicate code from JsonView and XmlView to SerializedView

ceeram · 2015-05-08T07:43:33Z

src/View/XmlView.php

-        }
-        parent::loadHelpers();
-    }
+    public $_responseType = 'xml';


why underscore public attribute?

Mistake in all 3 classes. Should be protected.

ceeram · 2015-05-08T07:45:12Z

src/View/JsonView.php

-            $response->type('json');
-        }
-    }
+    public $_responseType = 'json';


markstory · 2015-05-09T02:19:28Z

Looks good to me. This looks far safer than the original approach.

3.1 - Add support for `_serialize` = `true` for JsonView and XmlView

patrickconroy · 2015-09-29T19:39:18Z

Hey guys. With the way _serialize=true is working it doesn't seem possible to have it work when you have a single var.

$status = 1;
$this->set(compact('status') + ['_serialize' => true]);

If you do the above right now, you get this response, which isn't valid JSON:

However, if you have more than one var, it works as expected:

$status = 1;
$a = 'b';
$this->set(compact('status', 'a') + ['_serialize' => true]);

Returns:

{
    "status": 1,
    "a": "b"
}

The unexpectedness of that makes me think this is a bug, but it's possible that's as intended.

markstory · 2015-09-29T19:59:59Z

@patrickconroy Could you open a new issue with that?

patrickconroy · 2015-09-29T20:06:23Z

Sure np.

markstory · 2015-09-30T01:42:31Z

Thanks.

ADmad added the view label May 6, 2015

ADmad added this to the 3.1.0 milestone May 6, 2015

ADmad changed the title ~~Jsonview auto serialize~~ 3.1 - Jsonview auto serialize May 6, 2015

ceeram reviewed May 6, 2015
View reviewed changes

markstory added the enhancement label May 7, 2015

ADmad added 2 commits May 7, 2015 11:34

Allow setting _serialize to true.

e3d8e82

Doing so will serialize all available view variables.

Make serialization the default behavior for JsonView.

9893894

Set '_serialize' to false to turn off serialization.

ADmad force-pushed the jsonview-auto-serialize branch from 5a27b48 to 9893894 Compare May 7, 2015 06:11

Fix typo

7063459

markstory reviewed May 7, 2015
View reviewed changes

Switch back to template rendering as default.

e615ef8

ADmad mentioned this pull request May 7, 2015

Turn on serialization by default for JsonView/XmlView. cakephp/app#244

Merged

ADmad added 2 commits May 8, 2015 12:03

Add SerializedView.

32c1712

It contains common code used by view which return serialized data.

Add support for "_serialized" = true for XmlView.

62f7537

ceeram reviewed May 8, 2015
View reviewed changes

src/View/JsonView.php

$response->type('json');

}

}

public $_responseType = 'json';

Copy link

Contributor

ceeram May 8, 2015

Choose a reason for hiding this comment

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

and here

Fix visibility

016ecf2

Update docblocks

b8029c4

ADmad added a commit that referenced this pull request May 9, 2015

Merge pull request #6502 from cakephp/jsonview-auto-serialize

2b6821a

3.1 - Add support for `_serialize` = `true` for JsonView and XmlView

ADmad merged commit 2b6821a into 3.1 May 9, 2015

ADmad deleted the jsonview-auto-serialize branch May 9, 2015 04:00

ADmad mentioned this pull request May 9, 2015

Add info about new serialization feature cakephp/docs#2713

Merged

patrickconroy mentioned this pull request Sep 29, 2015

3.1 - _serialize = true unexpected results #7482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3.1 - Jsonview auto serialize #6502

3.1 - Jsonview auto serialize #6502

ADmad commented May 6, 2015

ceeram May 6, 2015

ADmad May 7, 2015

ceeram commented May 6, 2015

ceeram commented May 6, 2015

markstory commented May 7, 2015

ADmad commented May 7, 2015

markstory May 7, 2015

ADmad May 7, 2015

josegonzalez commented May 7, 2015

patrickconroy commented May 7, 2015

dereuromark commented May 7, 2015

ADmad commented May 7, 2015

markstory commented May 7, 2015

ADmad commented May 7, 2015

dereuromark commented May 7, 2015

ADmad commented May 7, 2015

dereuromark commented May 7, 2015

ADmad commented May 8, 2015

ceeram May 8, 2015

ADmad May 8, 2015

ceeram May 8, 2015

markstory commented May 9, 2015

patrickconroy commented Sep 29, 2015

markstory commented Sep 29, 2015

patrickconroy commented Sep 29, 2015

markstory commented Sep 30, 2015

3.1 - Jsonview auto serialize #6502

3.1 - Jsonview auto serialize #6502

Conversation

ADmad commented May 6, 2015

ceeram May 6, 2015

Choose a reason for hiding this comment

ADmad May 7, 2015

Choose a reason for hiding this comment

ceeram commented May 6, 2015

ceeram commented May 6, 2015

markstory commented May 7, 2015

ADmad commented May 7, 2015

markstory May 7, 2015

Choose a reason for hiding this comment

ADmad May 7, 2015

Choose a reason for hiding this comment

josegonzalez commented May 7, 2015

patrickconroy commented May 7, 2015

dereuromark commented May 7, 2015

ADmad commented May 7, 2015

markstory commented May 7, 2015

ADmad commented May 7, 2015

dereuromark commented May 7, 2015

ADmad commented May 7, 2015

dereuromark commented May 7, 2015

ADmad commented May 8, 2015

ceeram May 8, 2015

Choose a reason for hiding this comment

ADmad May 8, 2015

Choose a reason for hiding this comment

ceeram May 8, 2015

Choose a reason for hiding this comment

markstory commented May 9, 2015

patrickconroy commented Sep 29, 2015

markstory commented Sep 29, 2015

patrickconroy commented Sep 29, 2015

markstory commented Sep 30, 2015