Problem/Motivation

justinrainbow/json-schema is currently added as a core-dev dependency and pinned to the 5.x version
The 6.x version contains several important bugfixes that experience builder will need.

Steps to reproduce

Proposed resolution

Attempt to allow 5.x or 6.x version
The breaking changes between the two versions are here

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Support for justinrainbow/json-schema v6 has been added alongside v5. There are some breaking changes in v6, read the upgrade notes if you are using this package in custom code.

CommentFileSizeAuthor
#27 3516348-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3516348

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:

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Issue summary: View changes

longwave’s picture

As it's a dev dependency we could consider bumping the lockfile version to 6.x in 11.2 (while still allowing downgrades to 5.x), similar to us upgrading from PHPStan 1 to 2.

jackfoust’s picture

I believe this is also blocking composer to be updated past 2.8.6

dafeder’s picture

What would be the issue with allowing either on earlier Drupal versions? I'm trying to use 6.x in a module now and this would mean we'd basically have to wait until 11.1 is EOL to ship these features safely, right? I imagine Experience Builder would have the same issue...

mlubbers’s picture

Because justinrainbow/json-schema is currently added as a core-dev (11.6) dependency and pinned to the 5.x version, it causes updating composer from 2.8.6 to 2.8.8 to fail.

Composer -v wrongly reports composer version 2.8.8 (after composer self-update), using the "composer update composer/composer" command gives the message you're already using the latest version.

Running the command "composer outdated "still reports 2.8.6.
Version 2.8.6 is of course still mentioned in composer.json en -lock files.

I hope the next drupal/core-dev update allows to run justinrainbow/json-schema ^6.4 to resolve the issue.

longwave’s picture

Priority: Normal » Critical

Bumping to critical as this is preventing Composer from being updated.

nicxvan’s picture

This needs a rebase.

quietone’s picture

bbrala made their first commit to this issue’s fork.

bbrala’s picture

Rebased.

bbrala’s picture

Seems it has an undocumented change:

https://github.com/jsonrainbow/json-schema/issues/822

BaseContraint:

public function addError(?JsonPointer $path, $message, $constraint = '', ?array $more = null)

to

public function addError(ConstraintError $constraint, ?JsonPointer $path = null, array $more = []): void
longwave’s picture

Status: Active » Needs review

Note to committer: MR !11685 is the one to use so we stick to 5.x for now, the other one is to prove that the update passes. Later we may have to bump to 6.x as the default anyway to allow Composer to update.

bbrala’s picture

Status: Needs review » Reviewed & tested by the community

Went through the changes, these look good.

For a small moment i thought, why wouldnt we not just update the baseline? But that might be a hassle when switching between 5 and 6.

And in regards to contrib, it is pretty easy to support 5 and 6 for now, change is not that bad as you can see in this mr.

Anyways, all good, cool ^^

Edit: hmm, thinking, do we need a changerecord for this or are we communicating this in the parent issue?

longwave’s picture

Issue tags: +11.2.0 release notes

Don't think we need a change record, but we should add a line for this to the 11.2.0 release notes.

andypost made their first commit to this issue’s fork.

andypost’s picture

bbrala’s picture

Seems we have an issue left here after rebase:

------ ------------------------------------------------------------------------- 
  Line   core/tests/Drupal/Tests/Core/Theme/Component/ComponentValidatorTest.php  
 ------ ------------------------------------------------------------------------- 
  351    Call to an undefined static method                                       
         JsonSchema\ConstraintError::FORMAT_URL().                                
         🪪  staticMethod.notFound                                                
  355    Method JsonSchema\Constraints\BaseConstraint::addError() invoked with    
         4 parameters, 1-3 required.                                              
         🪪  arguments.count                                                      
 ------ ------------------------------------------------------------------------- 
bbrala’s picture

Never mind me, all good, that is the branch which uses 6.x to test. Adding ignore so tests run, but nothing to worry about. Keeping RTBC

bbrala’s picture

All green and happy after the rebase.

penyaskito’s picture

andypost’s picture

the only question when we can drop 5.x of it

longwave’s picture

@andypost Probably not until Drupal 12.

andypost’s picture

Component: single-directory components » base system
needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

bbrala’s picture

Status: Needs work » Reviewed & tested by the community

Updated lock file hash.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Given #17 we should add a release note snippet for 11.2.0 - probably pointing to https://github.com/jsonrainbow/json-schema/blob/master/UPGRADE-6.0.md to be helpful.

alexpott’s picture

Can be set to RTBC when done and I will prioritise.

longwave’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Added a note.

longwave’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 656836718c4 to 11.x and 4eb755b42fc to 11.2.x. Thanks!

  • alexpott committed 4eb755b4 on 11.2.x
    Issue #3516348 by bbrala, longwave, andypost, larowlan, jackfoust,...

  • alexpott committed 65683671 on 11.x
    Issue #3516348 by bbrala, longwave, andypost, larowlan, jackfoust,...
jackfoust’s picture

Can this be applied to 10.4.x?

andypost’s picture

Re #36 as a custom patch for sure

longwave’s picture

Version: 11.x-dev » 10.5.x-dev
Status: Fixed » Patch (to be ported)

Discussed with @catch, as this is a development dependency only and the compatibility changes are minor we think this could be backported to 10.5.x.

andypost’s picture

Status: Patch (to be ported) » Needs review

backported

bbrala’s picture

Status: Needs review » Reviewed & tested by the community

No weird changes, actual green tests in main MR and also the test mr with updates package. Nice :)

  • alexpott committed f6a31492 on 10.5.x
    Issue #3516348 by bbrala, andypost, longwave, larowlan, jackfoust,...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +10.5.0 release notes

Committed f6a3149 and pushed to 10.5.x. Thanks!

Status: Fixed » Closed (fixed)

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