Declare explicit runtime dependency on `justinrainbow/json-schema`

Yeah, just ran into that and @longwave told me to composer require drupal/core-dev which fixed that error.

Hm.

I was trying to stay closer to how JSON:API does it: better DX if you install this dev-only dependency.

But here, making it optional would make it impossible to validate that the data stored by XB:

• \Drupal\experience_builder\Plugin\Validation\Constraint\ValidComponentTreeConstraintValidator::validate() is XB's high-level validation logic, and it calls:
• \Drupal\Core\Theme\Component\ComponentValidator::validateProps(), which validates the given props values fit into a component's props.
• That logic looks like this:
  

    public function validateProps(array $context, Component $component): bool {
      // If the validator isn't set, then the validation library is not installed.
      if (!$this->validator) {
        return TRUE;
      }
  

IOW: it returns early if justinrainbow/json-schema is absent.

So AFAICT this indeed requires the XB module to explicitly depend on that library.

Thanks for surfacing this! 🙏

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 😱

Based on #4, if this does get in we should open a follow up to explore other options that aren't going to have performance regressions for unrelated parts of the site.

Instead of the JSON:API validation magically happening when the package is installed maybe we should move it to a feature flag module (that explicitly depends on the package as well).

Wow, VERY good call, @larowlan! 👏

@danielveza I don't see an alternative solution for this, because it's \Drupal\Core\Theme\Component\ComponentValidator that uses \JsonSchema\Validator … 🤔

I think #6 is the right call. Or better yet: have the JSON:API module add a , similar to core's recently added → ? 😊

You added it as development dependency, not a runtime dependency. 😅

P.S.: can you mark this issue next time when something is ready? 🙏

Pushed up an alternate approach based on the feature flag idea to 3469516-feature-flag.

It should hopefully prevent us from needing to add this as a runtime dep and stop the JSON:API preformance regressions outlined in #4.

That being said, I also agree with this in #7

>Or better yet: have the JSON:API module add a JSON:API development mode, similar to core's recently added Twig development mode?

We should open a core issue for this, it feels too easy to shoot yourself in the foot without realizing it and degrade your JSON:API performance. (As this issue has proved)

@danielveza I'm very sorry 😭… but the feature flag doesn't make sense here, only for JSON:API. For JSON:API, it's a nice-to-have, just to know about compliance of JSON:API responses with the JSON:API spec.

For XB, the JSON schema validation is critical: without it, \Drupal\experience_builder\Plugin\Validation\Constraint\ValidComponentTreeConstraintValidator::validate() becomes pretty much pointless.

That's AFAICT also what @longwave proposed in #6: that it's Drupal core's JSON:API functionality that should be doing that validation behind a feature flag.

Am I missing something here? Can you confirm, @longwave? 🙏

Yeah I meant that we should change core so JSON:API validation is explicitly enabled with a feature flag. I wonder even if it should just be a contrib module and the feature removed from core directly, but that is a product manager decision. Will open a core issue to discuss.

Opened 📌 Move JSON:API validation to a feature flag or separate module Active to discuss the core change.

For the time being, I think we should land MR!247 in XB, marking RTBC for that.

wim leers → closed merge request !249

NW for https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

After that's fixed, will happily merge :)

Made changes.

wim leers → credited f.mazeikis → .

2 out of 3 needed MR approvals, but @longwave approved it here at #18, so bypassing the GitLab CODEOWNERS check to be pragmatic… 👍

My POC didn't take long so it's ok that it didn't make it in.

The reason I thought a feature flag here could be valuable is that we have no guarantee that the core issue is going to get in before XB is considered production ready or early adopters start using it and I'd like to avoid future devs having pain when trying to track down what is most likely a quite annoying performance regression to figure out.

Probably the softer approach for XB rather than the feature flag that removes it is a hook_requirements implementation that runs if JOSN:API is also enabled. I'll open a ticket.

@danielveza That's a relief! 😮‍💨

I'd like to avoid future devs having pain when trying to track down what is most likely a quite annoying performance regression to figure out.

That's totally fair.

Probably the softer approach for XB rather than the feature flag that removes it is a hook_requirements implementation that runs if JOSN:API is also enabled. I'll open a ticket.

I was going to suggest docs but … this is far better! 👏🤩 Thank you! 🙏

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