Move JSON:API validation to a feature flag or separate module

Actually given that this is purely a developer-facing feature I am not even sure this is a product manager decision, removing the tag again.

wim leers → credited larowlan → .

Quoting @larowlan:

FYI if we make this a production requirement, we should add a hook_requirements warning people with jsonapi module and assertions enabled that their performance will go through the floor because of that module's response validator.

I've been asked to audit sites for performance and found that combo in production 😱

— #3469516-4: Declare explicit runtime dependency on justinrainbow/json-schema →

As a JSON:API maintainer: +1 for this.

Hmm, automatic discovery is kinda unsettling indeed. I agree that needs to go. I wonder if we will make things less stable if we stop validating. Especially in tests thi has merit. Altough, if performance on this is very bad (have not seem any numbers in the parent) we might want to choose and disable in anyways.

I do kinda agree on ussing a flag perhaps as twig, seems reasonable. Wonder though if we need to change ci then also to enable that. Something like parameters.jsonapi.validate_schema comes to mind then.

In order to be able to move forwards relatively quicky, this could first just be a parameter defaulting to false if not passed. Then we can consider (and perhaps implement) developer mode in a follow up.

In other news; I was also thinking if this would mean problems in ✨ Generate JSON schema for content entity types Needs review but i think that is all fine.

Wondering, would this same issue exist for:

Drupal\Core\Theme\Component\ComponentValidator

That kinda does the same.

  /**
   * Sets the validator service if available.
   */
  public function setValidator(?Validator $validator = NULL): void {
    if ($validator) {
      $this->validator = $validator;
      return;
    }
    if (class_exists(Validator::class)) {
      $this->validator = new Validator();
    }
  }

Test run results. Kinda weary to do a full conclusion since variance is so high.

Run analytics timeing of step_script	Disable 1	Disable 2	Enable 1	Enable 2
🖱️️️ PHPUnit Functional Javascript 1/3	03:50		02:49		04:03		03:33
🖱️️️ PHPUnit Functional Javascript 2/3	03:49		03:29		04:12		03:58
🖱️️️ PHPUnit Functional Javascript 3/3	02:56		02:38		04:03		03:12
🌐️️ PHPUnit Functional 1/8		03:32		02:52		04:29		03:54
🌐️️ PHPUnit Functional 2/8		03:41		02:42		04:10		02:36
🌐️️ PHPUnit Functional 3/8		03:36		03:21		05:22		02:44
🌐️️ PHPUnit Functional 4/8		03:05		03:11		04:20		02:56
🌐️️ PHPUnit Functional 5/8		03:38		02:47		03:27		03:37
🌐️️ PHPUnit Functional 6/8		02:59		03:20		04:22		03:43
🌐️️ PHPUnit Functional 7/8		02:42		02:33		03:59		03:24
🌐️️ PHPUnit Functional 8/8		02:45		02:45		04:56		03:06
Total time taken in functional		36:33		32:27		47:23		36:43

So question then becomes. Do we really want this, or perhaps only enable this in testing for a specific node?

Think this should fail since the validation is off by default now. Lets see if that is the case.

Ok it seems we only have unittests for the code. That is fine, i did a booboo on the unit test arguments though, so lets fix that. Validation should be disbled by default now at least.

Think this is fine, first time doing a pattern like this, so not sure if this is the right way.

Adding the extra contructor argument shoiuld be fine. We could also do this without a default value if preferred, but this way chances of breaking something seem smaller.

Code change looks fine but since it's adding a default service probably needs a change record. Maybe release notes? not 100% on the second.

CR drafted, child issue for #6 created in 📌 aInvestigate if ComponentValidator should have soft dependency on justinrainbow/json-schema Active

Rebased and small tweak to test method order and comment. All ready.

Thanks!

CR reads well to me, clearly defines the need and code example makes it look clean.

Ran test-only feature

1) Drupal\Tests\jsonapi\Unit\EventSubscriber\ResourceResponseValidatorTest::testValidateResponse with data set #5 (Symfony\Component\HttpFoundation\Request Object (...), Drupal\rest\ResourceResponse Object (...), true, 'Response validation disabled ...valid.', false)
Response validation disabled doesn't flags empty array as invalid.
Failed asserting that false is identical to true.
/builds/issue/drupal-3472008/core/modules/jsonapi/tests/src/Unit/EventSubscriber/ResourceResponseValidatorTest.php:81
FAILURES!
Tests: 6, Assertions: 6, Failures: 1.
Exiting with EXIT_CODE=1

Which shows coverage.

Rest of the code looks good and believe this is good to go.

Added a question about the new container parameter.

Fair point. Will do

I'm also assuming that this can then be overridden still in a services.local.yml, but haven't checked - we should update the change record to tell people how to reenable the feature.

sumit saini → made their first commit to this issue’s fork.

Added changes as per #17. Also, fixed flag name mismatch in files.
For #19, I have tested and confirm that it is possible to override this container parameter in services.local.yml with these changes.