Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

StatusFileSize
new657 bytes

I'm curious which tests, if any, will fail with this.

Status: Needs review » Needs work

The last submitted patch, 2: jsonapi-request-format.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

effulgentsia’s picture

Hm, so the answer seems to do with page cache expectations. See:

Something about this feels off, but I'm not exactly sure what yet. I'll come back and comment here when I have something useful to say about it.

effulgentsia’s picture

Title: Why is FormatSetter (Accept header negotiation) necessary? » Add code comments for why FormatSetter middleware is necessary
Assigned: Unassigned » effulgentsia
Category: Support request » Task

Now that I thought about it some more, I agree with it. But, I'd like to add code comments to explain it to future people who wonder the same.

effulgentsia’s picture

I haven't gotten to this yet, but in the meantime I opened #3028501: Enable header-based proactive content negotiation with optimizations and opt-outs available. as a core issue.

wim leers’s picture

wim leers’s picture

Title: Add code comments for why FormatSetter middleware is necessary » [PP-1] [upstream] Remove FormatSetter once core's need for it disappears
Status: Needs work » Postponed
Issue tags: +API-First Initiative
StatusFileSize
new2.2 KB

Oh, that sounds lovely! 👏 I think we'd all love to remove this middleware!

I fixed the test failures over at #3029373-6: PageCache::getCacheId() sometimes uses different cache IDs during get and set, causing unnecessary cache misses for some requests.


With both that core patch applied and the JSON:API patch that I attached here, tests are mostly passing:

Testing Drupal\Tests\jsonapi\Functional\NodeTest
.......F...                                                       11 / 11 (100%)

Time: 1.95 minutes, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testPostIndividual
Failed asserting that Array &0 (
    0 => 'text/html; charset=UTF-8'
) is identical to Array &0 (
    0 => 'application/vnd.api+json'
).

So it looks like this will be viable :)

wim leers’s picture

StatusFileSize
new908 bytes
new3.06 KB

I dug in and that failing test is legitimate. Some error responses happen before or during routing, and at that time the JSON:API format hasn't been negotiated yet. So … you get a HTML response.

This may be acceptable.

effulgentsia’s picture

Title: [PP-1] [upstream] Remove FormatSetter once core's need for it disappears » [PP-1] [upstream] Move FormatSetter from middleware to a route filter, removing conditionality on Accept header
StatusFileSize
new3.84 KB

Some error responses happen before or during routing, and at that time the JSON:API format hasn't been negotiated yet.

Well, let's fix that :) How's this?

effulgentsia’s picture

StatusFileSize
new3.87 KB
new278 bytes

Oops. Forgot to include a local change that I had in the #11 patch. Here it is.

wim leers’s picture

Title: [PP-1] [upstream] Move FormatSetter from middleware to a route filter, removing conditionality on Accept header » [upstream] Move FormatSetter from middleware to a route filter, removing conditionality on Accept header
Status: Postponed » Needs review
wim leers’s picture

Oh, right, this can never be committed to JSON:API 2.x since it must work for 8.5+8.6+8.7. If we're lucky, we could get #3029373: PageCache::getCacheId() sometimes uses different cache IDs during get and set, causing unnecessary cache misses for some requests backported to 8.6. But it'll never get backported to 8.5.

So, we can only do this as part of #3015325: [ignore] support issue for the core patch AFAICT.

The last submitted patch, 11: 3027980-11.patch, failed testing. View results

The last submitted patch, 9: 3027980-9.patch, failed testing. View results

The last submitted patch, 10: 3027980-10.patch, failed testing. View results

wim leers’s picture

For the reasons given in #14, this passed tests on 8.7, but not on 8.5 or 8.6.

Thoughts?

effulgentsia’s picture

Status: Needs review » Closed (duplicate)
effulgentsia’s picture

wim leers’s picture