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.

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 JSON:API development mode, similar to core's recently added Twig development mode? 😊

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

P.S.: can you mark this issue Needs review 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). If this approach gets approved and JSON:API gets a "dev mode", we could consider deprecating this feature flag.

@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? 🙏

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

After that's fixed, will happily merge :)

Made changes.

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. 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! 🙏