ResourceResponse should treat \stdClass as an array. Otherwise we receive an exception because we cannot find a normalizer.

I found this problem while working on #2403307: RPC endpoints for user authentication: log in, check login status, log out because I need to use a stdClass object with the current user and session data.

Patch attached.

CommentFileSizeAuthor
#12 resourcehandler_can_t-2427811-12.patch637 bytesneclimdul
#4 treat_stdclass_as_an-2427811-4.patch1008 bytesisholgueras
rest-stdclass.patch670 bytesmarthinal
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Is this really a major issue? It sounds more like a normal task, for a small developer experience.

I guess in general we should have a test as well.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented
Priority: Major » Normal
Status: Needs review » Needs work
Issue tags: +Needs tests

Agreed it's not major, but I'd like to see it. A quick test and we should be good to go.

isholgueras’s picture

isholgueras CreditAttribution: isholgueras commented
Assigned: Unassigned » isholgueras

Working on it.

isholgueras’s picture

isholgueras CreditAttribution: isholgueras commented
FileSize
treat_stdclass_as_an-2427811-4.patch1008 bytes

The previous patch doesn't apply right. So I reroll and now apply right. This is the new patch with the same functionality.

Now I will write the tests.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review

To run the test you need to change the status to needs review.

isholgueras’s picture

isholgueras CreditAttribution: isholgueras commented
Status: Needs review » Needs work

working on tests

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: isholgueras » Unassigned

Tests still needed.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Title: Treat \stdClass as an array for serialization. » ResourceHandler can't serialize a stdClass

I think this might be the wrong approach and actually puts the solution before the problem.

It happens to work because currently ResourceHandler can't handle serializing "empty" objects and arrays because they evaluate to false which is a bug (#2661642: ResourceResponse can't serialize empty array). Once that is fixed, this fails for the case of an empty object because
json_encode((array) new \stdClass()) != json_encode(new \stdClass())

Ideally the serializer would know how to serialize a bare class the same way it knows how to serialize an array. I don't know the code well enough to know if that is feasible.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: ResourceHandler can't serialize a stdClass » [PP-1] ResourceHandler can't serialize a stdClass
Status: Needs work » Postponed

Let's postpone this on that issue then.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs issue summary update

It's not clear to me nor to damiankloip why exactly this is necessary. Could you provide a concrete use case/example in the IS?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [PP-1] ResourceHandler can't serialize a stdClass » ResourceHandler can't serialize a stdClass
Status: Postponed » Needs work

#2661642: ResourceResponse can't serialize empty array landed, this is unblocked.

This still needs test coverage.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Version: 8.0.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
resourcehandler_can_t-2427811-12.patch637 bytes

Tests, no implementation though. Should be easier to implement now that the previous patch fixed that ability to post empty data.

Status: Needs review » Needs work

The last submitted patch, 12: resourcehandler_can_t-2427811-12.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

This seems really related to #2419825: Make serialization_class optional

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented

Yeah sort of related. Its would probably be broken, though I'm not sure were, if it provided objects instead of an array containing scalars. Arrays and scalars work fine.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Closed (works as designed)

There's no more need to use \stdClass. You should always use actual classes/interfaces. Otherwise… just use array. And as #15 says: arrays work fine.

I don't see why we would want to support \stdClass.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
CreditAttribution: nikunjkotecha as a volunteer and at QBurst commented

One reason to support stdClass explained in this article

We are supporting this today using custom normaliser, it would be great to have support in CORE.