Since #2751325: All serialized values are strings, should be integers/booleans when appropriate has landed, but the output from JSONAPI still has Integer serialized as String. Below are steps to reproduce:

1. Fresh install Drupal 8.3.0-rc1
2. Download and Enable jsonapi module
3. Create an article at node/1
4. Access it from JSONAPI 

curl -X GET -H "Cache-Control: no-cache" “***********/jsonapi/node/article?_format=api_json"

The result has "nid", "vid", and "promoted" values in String

CommentFileSizeAuthor
#10 2858023-10.patch982 bytesWim Leers
#10 interdiff.txt797 bytesWim Leers
#4 2858023-4.patch779 bytesdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

skyredwang created an issue. See original summary.

skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang at INsReady commented 
<e0ipso> We are not calling the serializer on the scalars
skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang at INsReady commented
Parent issue: » #2829327: Review JSON API module for core-readiness
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Active » Needs review
FileSize
2858023-4.patch779 bytes

Let's see what happens.

Wim Leers’s picture

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

This is why we need the JSON API test coverage to expand significantly.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

This is why we need the JSON API test coverage to expand significantly.

I don't think that would have helped here, but I would love to see the test coverage increase.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs review » Needs work

@skyredwang can we have testing for this? :-D

skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang at INsReady commented

I manually tested #4 patch, it works.

I don't know how to write automated tests in Drupal 8 yet. There is a big gap between https://phpunit.de/getting-started.html and https://www.drupal.org/docs/8/phpunit . I haven't got time to figure it out. Maybe one day, someone will write a simple PHPunit test for Drupal 8 tutorial or point me to one, then, I can get started on writing automated tests.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#6: it would have. See how #2751325: All serialized values are strings, should be integers/booleans when appropriate was forced to update functional test coverage. Functional tests assert the response bodies and hence would've covered this.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
Related issues: +#2751325: All serialized values are strings, should be integers/booleans when appropriate
FileSize
interdiff.txt797 bytes
2858023-10.patch982 bytes

BTW, I'm not saying we should block committing this on adding test coverage. The existing JSON API test coverage focuses not on concrete serializations/normalizations, so it makes sense that this patch is not causing any tests to fail.

I do think #4 should get the same comment as #2751325: All serialized values are strings, should be integers/booleans when appropriate added to \Drupal\hal\Normalizer\FieldItemNormalizer::normalize().

Status: Needs review » Needs work

The last submitted patch, 10: 2858023-10.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I only added comments. How could this possibly fail? :O

Oh, wait. Due to a change in Drupal 8.4, JSON API HEAD is just failing on 8.4.x. See #2855693-21: Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber for the solution.

skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang at INsReady commented
Status: Needs work » Needs review

  • e0ipso committed 8df3e73 on 8.x-1.x authored by skyredwang 
    fix(Serialization): Type cast supported JSON format (#2858023 by Wim...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs review » Fixed

Thanks all. This is committed and it's going to be the HEAD of our new beta release!!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Yay!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.