As suggested by @dawehner in #2855693-22: Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber.1.

CommentFileSizeAuthor
#13 2859207-13.patch20.57 KBwim leers
#13 interdiff.txt689 byteswim leers
#8 2859207-8.patch20.61 KBwim leers
#6 2859207-6.patch20.6 KBwim leers

Comments

Wim Leers created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Thanks for taking the time to create this!

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Title: [PP-1] Move Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::validateResponse() to its own subscriber » Move Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::validateResponse() to its own subscriber
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Active
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2917260: Validate against specific JSON Schemas if Schemata is present

Again relevant with #2917260: Validate against specific JSON Schemas if Schemata is present.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
Related issues: +#2941685: Port parameter based resource config from REST module
StatusFileSize
new 2859207-6.patch20.6 KB

Done.

Together with #2941685: Port parameter based resource config from REST module, this will make sure that \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber matches \Drupal\rest\EventSubscriber\ResourceResponseSubscriber as closely as possible, hence opening the door to moving it into the serialization module in the future.

Rather than one very complex event subscriber, we now have two simple ones, one of which will disappear in the future!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Needs review » Needs work

#2941685: Port parameter based resource config from REST module landed, this needs to be rebased.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2859207-8.patch20.61 KB
gabesullice’s picture

gabesullice
Primary language English
commented
Issue tags: +API-First Initiative

Looks really good. Just one thing:

+++ b/src/EventSubscriber/ResourceResponseValidator.php
@@ -0,0 +1,257 @@
+   * Serializes ResourceResponse responses' data, and removes that data.

Outdated comment.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

RTBC, the one test failure is due to a required update to HEAD because of: #2950129: Add helpful reason for 'update' and 'delete' access not being allowed to MediaAccessControlHandler

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Whoops, actually still needs work because of the comment, then it can be RTBC.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Related issues: +#2969493: MediaTest::testPatchIndividual() and MediaTest::testDeleteIndividual() failing on 8.6

For #10, I created #2969493: MediaTest::testPatchIndividual() and MediaTest::testDeleteIndividual() failing on 8.6.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt689 bytes
new 2859207-13.patch20.57 KB

Addressed #9.

Will re-RTBC once this passes against 8.6, i.e. once #2969493: MediaTest::testPatchIndividual() and MediaTest::testDeleteIndividual() failing on 8.6 lands.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#2969493: MediaTest::testPatchIndividual() and MediaTest::testDeleteIndividual() failing on 8.6 just landed, queuing a 8.6 test of #13.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Green on both 8.5 & 8.6!

Wim Leers credited dawehner.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Crediting @dawehner because he suggested it (see IS).

  • Wim Leers committed 6e33f68 on 8.x-1.x 
    Issue #2859207 by Wim Leers, gabesullice, dawehner: Move Drupal\jsonapi\...
  • Wim Leers committed 5ddf31a on 8.x-2.x
    Issue #2859207 by Wim Leers, gabesullice, dawehner: Move Drupal\jsonapi\...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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