Improve Dynamic Page Cache header assertions in JSON:API tests

Code looks good, al threads pretty much awnsered. I wonder if we should create a followup for the todo around max-age so its at least in the queue.

All good, you removed the comment which is fine. :)

Thanks @bbrala! :)

❯ git rebase origin/11.x
Auto-merging core/.phpstan-baseline.php
Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
CONFLICT (content): Merge conflict in core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
error: could not apply 5ae7c67a39... Introduce generateDynamicPageCacheExpectedHeaderValue
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".

Rebase only impacted ResourceTestBase but due to this inherited problem more changes became necessary. all changes since the previous RTBC on ResourceTestBase are here: https://git.drupalcode.org/project/drupal/-/merge_requests/9465/diffs?st...

Moving back to needs review due to additional changes performed after rebase.

Reading through the MR I feel like I'm missing something. We're changing the default value from FALSE to NULL and then the subsequent check for said variable is also updated accordingly. What was the problem with it being FALSE rather than NULL?

A recently merged changed the original behavior in the assert function, including what false means. More details are here:

https://git.drupalcode.org/project/drupal/-/merge_requests/9465#note_386330

So I decided it is better if I clean this up as well to avoid future confusion and replace FALSE|STRING with NULL|STRING.

Also, it looked like page cache became more chatty in X-Drupal-Cache since that change and probably this was the reason behind those changes in ResourceTestbase originally, but they weren't aligned with the original design,see my previous comment.

Okay, thanks for clarifying.

Left a small question.

Went through the whole thing and changes are looking good.

It does make me dream of a less magic way of testing jsonapi again. But that mountain is kinda daunting.

Thanks for the review!

Let me start with a no-brainer rebase to ensure this MR remains up to date.

❯ git rebase origin/11.x
Successfully rebased and updated refs/heads/3473374-improve-resourcetestbase.

No conflicts.

This ready for review?

Last comment was fixed. So as per #15, this is RTBC for me. Makes quite a few things a bit more consistent. Although this test class does hurt my eyes.

larowlan → closed merge request !9465

I've committed this to 11.x and have asked other committers for an opinion on backporting it into the RC for 10.4/11.1

Discussed with @longwave who agreed this was fine to backport to 11.1.x and 10.4.x so have done so.

Exciting news! Thanks for everyone who got involved, this is an important step to fix the dependent issue.

🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work can be only backported to 11.0.x is this also gets bacported.

As a task/feature request this one isn't eligible for further backport

Discussed with other committers and backporting this was seen as important so we can keep tests consistent and unblock backporting 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work

larowlan → changed the visibility of the branch 3473374-improve-resourcetestbase to active.