Overview

#3469516: Declare explicit runtime dependency on `justinrainbow/json-schema` made justinrainbow/json-schema a runtime requirement. It was raised by @larowlan in #4 of that issue that it will impact JSON:API performance.

#3472008: Avoid JSON:API schema validation by default when assert is enabled will resolve that, but we should be nice and add a hook_requirements check to warn users about this until the core issue is committed.

Proposed resolution

Add a requirements check
Include a reference to the core issue so the hook_requirements can be removed when the core issue lands

User interface changes

CommentFileSizeAuthor
#5 xb-jsonapi-warning.png23.25 KBakhil babu

Issue fork experience_builder-3472193

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

danielveza created an issue. See original summary.

akhil babu’s picture

akhil babu
he/him
commented
Assigned: Unassigned » akhil babu
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Add hook_requirements warning for sites with JSON:API » Add hook_requirements() warning for sites with JSON:API
Related issues: +#3469516: Declare explicit runtime dependency on `justinrainbow/json-schema`, +#3472008: Avoid JSON:API schema validation by default when assert is enabled

Thanks for creating this, @danielveza! 🙏

akhil babu opened merge request !260

akhil babu’s picture

akhil babu
he/him
commented
StatusFileSize
new xb-jsonapi-warning.png23.25 KB

Added the warning.

/files/issues/2024-09-05/xb-jsonapi-warning.png

akhil babu’s picture

akhil babu
he/him
commented
Assigned: akhil babu » Unassigned
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Add hook_requirements() warning for sites with JSON:API » Add hook_requirements() warning for sites with JSON:API, error if assertions are enabled
Component: Data model » Code
Status: Needs review » Needs work
Issue tags: +Performance

Almost ready! 👍

akhil babu’s picture

akhil babu
he/him
commented
Status: Needs work » Needs review

Thanks @wim leers, I've used the same logic in \Drupal\experience_builder\EventSubscriber\ApiMessageValidator::isProd() to check if assertions are enabled.

  • Should we change this to a common function? (ApiMessageValidator::isProd()) is a private method
  • Should the priority be changed to REQUIREMENT_ERROR if assertions are enabed?
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

#8:

  • Possibly. Let's wait for there to be 3 occurrences, to avoid premature abstraction. For now, please add an @see to the other place.
  • Yes 🙏
akhil babu’s picture

akhil babu
he/him
commented
Assigned: Unassigned » akhil babu
akhil babu’s picture

akhil babu
he/him
commented
Assigned: akhil babu » Unassigned
Status: Needs work » Needs review

Thanks @wim leers. Committed the changed

wim leers credited larowlan.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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