Skip to content

Upgrade json-schema to v6 #12348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 2, 2025
Merged

Upgrade json-schema to v6 #12348

merged 4 commits into from
Apr 2, 2025

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Mar 24, 2025

Fixes #12345

cc @DannyvdSluijs there are still some issues I couldn't figure out, maybe you have a clue?

In JsonFile::validateJsonSchema it passes a validation request like this:

class stdClass#7961 (4) {
  public $$ref =>
  string(76) "file:///var/www/composer/src/Composer/Json/../../../res/composer-schema.json"
  public $additionalProperties =>
  bool(false)
  public $required =>
  array(2) {
    [0] =>
    string(4) "name"
    [1] =>
    string(11) "description"
  }
}

That used to work well, combining the schema file with some additional constraints about additional props and required props.. But not these seem to be ignored.

The other thing that seems to have broken from v5 to v6 is that we cannot pass arrays in anymore to be validated, it needs a (object) cast to work.. That seems harmless enough but was surprising as it's not mentioned anywhere.

@private-packagist Private Packagist
Copy link

private-packagist bot commented Mar 24, 2025 •
edited
Loading

composer.lock

Package changes

Package Operation From To About
marc-mabe/php-enum add - v4.7.1 view code - License: BSD 3-Clause "New" or "Revised" License
justinrainbow/json-schema upgrade 5.3.0 6.3.1 diff
composer/class-map-generator downgrade 1.6.1 1.6.0 diff

Settings · Docs · Powered by Private Packagist

@Seldaek Seldaek added this to the 2.8 milestone Mar 24, 2025
@DannyvdSluijs
Copy link

DannyvdSluijs commented Mar 25, 2025 •
edited
Loading

I've spent some time looking into the issue and did find some things:

  • Ignore $ref siblings & abort on infinite-loop references (#437)

Correct in version 5 there was a misinterpretation of what the $ref keyword should do. This was fixed in jsonrainbow/json-schema#437. I'll look into this to find a solution, first idea is to use the allOf keyword to extend a $ref with some additional information.

The other thing that seems to have broken from v5 to v6 is that we cannot pass arrays in anymore to be validated, it needs a (object) cast to work.. That seems harmless enough but was surprising as it's not mentioned anywhere.

This is a difficult one apparently array support had found its way into the codebase. Object are more natural to JSON as the concept of an associative array only exists in PHP and not in JSON. This has side effects when casting object to array and vice-versa. It wasn't mentioned and therefor we are trying to fix (if possible) any issues with array data in version 6. But I'm also inclined to deprecate array data/schemas in version 6, removing it in version 7. So objects should be the way to go.

I'm a bit busy at the moment so I'm afraid this might take so time to continue investigation.

@Seldaek
Copy link
Member Author

Seldaek commented Mar 25, 2025

No rush at all, just wanted to get the ball rolling, and thanks for the context!

stof
stof reviewed Mar 25, 2025
View reviewed changes
Seldaek
Seldaek commented Mar 25, 2025
View reviewed changes
src/Composer/Json/JsonFile.php
$schemaData->required = [];
} elseif ($schema === self::STRICT_SCHEMA && $isComposerSchemaFile) {
if ($schema === self::STRICT_SCHEMA && $isComposerSchemaFile) {
$schemaData = json_decode((string) file_get_contents($schemaFile));
Copy link
Member Author

@Seldaek Seldaek Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So getting rid of the $ref and instead loading the whole file in memory + adding the props below seems to work well.. Does anything speak against that do you think @DannyvdSluijs ?

Note that I tried to hack it with allOf but that didn't work as the branch of the allOf with the require failed saying name was required but also not defined, as it was defined only in the other branch. I guess this might be considered a bug but it's also a weird edge case.

@private-packagist Private Packagist
Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Seldaek Seldaek merged commit fc5a3dd into composer:main Apr 2, 2025
20 checks passed
@xabbuh xabbuh mentioned this pull request Apr 4, 2025
Closed
@stof
Copy link
Contributor

stof commented Apr 4, 2025

The Symfony CI is broken when running on a 32bits PHP runtime with this change: https://github.com/symfony/symfony/actions/runs/14262540647/job/39977141591#step:5:23. I reported it at jsonrainbow/json-schema#816

kayw-geek pushed a commit to kayw-geek/composer that referenced this pull request Aug 14, 2025
@Seldaek @kayw-geek
Upgrade json-schema to v6 (composer#12348)
3f18597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@stof stof stof left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.8
Development

Successfully merging this pull request may close these issues.

Update for justinrainbow/json-schema
3 participants
@Seldaek @DannyvdSluijs @stof