Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses

Here's a patch I created with the following commands:

cd core
composer require --dev --no-update --sort-packages justinrainbow/json-schema
cd ..
composer update justinrainbow/json-schema

The --sort-packages puts it in the correct place by alphabet, but also causes a re-ordering of a couple other libraries that aren't currently alphabetized. Do we need to spin that other re-ordering to a separate issue, or is this patch small and simple enough to just do it all in one shot here?

The IS says:

Other dependencies it'd add, if any:
"require": {
...
"marc-mabe/php-enum": "2.3.1",
"icecave/parity": "1.0.0"
}

I don't see those being added in #3's composer.lock nor referenced in https://github.com/justinrainbow/json-schema/blob/master/composer.json. Were those dependencies of earlier versions but no longer? If so, then we can remove them from the IS?

I see. Yeah, that was committed to Master, but not to the 5.x.x branch, in https://github.com/justinrainbow/json-schema/commit/9d290a580a2149ac41fa....

It was explicitly rejected for backport in https://github.com/justinrainbow/json-schema/pull/558 as:

* #518 Strict Enum/Const Object Checking - out of scope for 5.x.x

Rebased.

1. The lack of info for a lot of our checks for adding a library is a bit worrying; however, since it is only a dev dependency, I'm comfortable being fairly forgiving. E.g., we've had issues in the past with dev dependencies having security policies incompatible with ours, and this resulted directly in a change to Drupal.org to no longer packaging dev dependencies and a PSA warning site owners that they should also not include dev dependencies on public-facing sites. So the risk there has been mitigated somewhat.
    

2. We should do a similar evaluation for the dependencies it would add as well. Per @effulgentsia they're dependencies for the upcoming branch, not the current one, but I think we should take it into account too since that affects future maintainability etc.
    

3. For maintainership:

   • https://github.com/justinrainbow/json-schema/graphs/contributors
   • https://github.com/justinrainbow/json-schema/releases

   It does make me a little nervous as it looks like the recent activity is fairly low and it seems to be just the one person making releases. It worries me when we depend on projects much smaller than ours that have a high bus factor. Given what I see there, I would ask: If the package were removed, or had an unresolved security issue, or stopped being maintained, would we be comfortable forking it and maintaining it ourselves, or removing it? What would the impact be if it were not available as a dev dependency?

   Or, put another way, how hard would it be to remove the dependency in an emergency situation? (E.g. leftpad or a security issue or whatever.) What would break?
    

4. Finally, for things like security policies which we're not able to locate, a simple thing we can do is reach out to the maintainers in their github queue and simply ask what their security, release, and BC policies are. Is anyone able to do that? (I wish we had a template for inquiries about it but I don't have a past example handy.)

   Depending on the answers to point 3, getting this feedback could be a "should-have" that doesn't need to block commit.

Thanks!

Thanks @gabesullice!

Will do. Actually, maybe can we leave that for now and do it when/if we update to version 6 (when it's released)?

I do think we should look into it now, because this affects the upgrade path for D9 and etc. However, we can just do a quick high-level pass for the three packages given the answer to #13.3; it sounds like any risks are well-contained.

it sounds like any risks are well-contained

In terms of jsonapi, I agree. If we had to suddenly remove the vendor library, we could just remove the tests that make use of it. However, is there any concern that if it's a require-dev dependency of core, then contrib modules might start relying on it too? E.g., some JSON related module (not necessarily related to JSON:API, just something json related) that writes a test that uses the library? Then if we remove the library, those modules' tests start breaking.

Probably the "correct" thing would be that any non-core modules that write tests that rely on some vendor package should specify that dev dependency in their own composer.json file, and not implicitly rely on core providing it. But I don't know how we can ensure that, and it's not like we require or expect contrib modules to explicitly list phpunit in their composer.json files, so there's already precedent for contrib modules to implicitly rely on what core provides.

I would still treat removing the dependency as a disruption and internal BC break. Just saying that, given how the package is used, removing it would be a manageable contingency plan if something with the package maintenance goes sideways, in a way that wouldn't be feasible for a core runtime dependency or something essential like PHPUnit. And that makes me reasonably comfortable with adding the dependency despite the risks of it being a smaller package without formal release policies.

Response from the maintainer an hour ago:

Hey, @gabesullice.

Thanks for reaching out.

There are no official (i.e., written) policies for this package.

For security releases, we would do the best effort to patch affected versions. Most fixes and updates are backported, but we don't have branches for versions older than v5.

There is no regular release cadence. Releases are mostly PR-driven and covered in PR discussions.

Yes, the use of semver is in place to provide BC assurances. There is no guarantee, per se, but I think the conventional SDLC provides general coverage.

I don't think the questions are over-the-top, and we are honored to be considered for another large project. Simply put, this is a 100% community-driven package, and certainly the labor of love for @erayd and @shmax.

@gabesullice and I assessed https://github.com/marc-mabe/php-enum and https://github.com/marc-mabe/icecave/parity. Neither have security or BC policies, @gabesullice opened similar issues for those: https://github.com/marc-mabe/php-enum/issues/112 + https://github.com/IcecaveStudios/parity/issues/15 + https://github.com/IcecaveStudios/repr/issues/4.

The issue summary has been updated accordingly.

There are two things worth calling out:

1. Also, justinrainbow/json-schema depends on marc-mabe/php-enum 2.x, but it seems only 3.x is actively maintained. Fortunately, there is https://github.com/justinrainbow/json-schema/pull/464#issuecomment-46934... to make justinrainbow/json-schema support for both 2.x and 3.x. We also asked mac-mabe/php-enum whether 2.x still gets security releases at https://github.com/marc-mabe/php-enum/issues/112#issuecomment-469350913. Note that the 3.x branch requires PHP >=5.6.
2. Also, justinrainbow/json-schema depends on icecave/parity 1.0.0, but as of November 2018, a 2.0.0 was released, which requires PHP >=7.1. So here too, I asked whether they'd be open to do security releases against 1.0.0: https://github.com/IcecaveStudios/parity/issues/15#issuecomment-469356962.

Thanks @Wim Leers! The prompt response from the json-schema maintainers is heartening. I posted a followup question asking about coordinated disclosure.

Adding a couple links to the IS for the other packages.

Based on https://github.com/IcecaveStudios/parity/issues/15 I do not think we should add the Icecave dependencies to our dependency tree. The current patch doesn't (see above WRT how these are dependencies in the master branch that were not backported); however, we could run into trouble in the future. Maybe we should report the above in the json-schema issue queue and suggest they consider an alternate approach?

I do not think we should add the Icecave dependencies to our dependency tree.

+1

The current patch doesn't

Right, because in:

+++ b/core/composer.json
@@ -63,7 +63,8 @@
+        "justinrainbow/json-schema": "^5.2"

The ^ keeps us on 5.x, so even when 6.x has a release, we won't update to it until we explicitly change this line.

however, we could run into trouble in the future

Per #17, the json-schema maintainer says he'll make a best effort to security patch 5.x even when 6.x is out. That might turn out to not be true, or not be true forever, but it's something. Therefore, I think it makes sense to for now:

• Assume we can stay on 5.x for a while.
• Per #20, collaborate upstream to get 6.x to a state where we could update to it if we wanted to.
• If we fail at the above and 5.x goes EOL at some point, then we can fall back to removing the library per #16 and lose some of our test coverage until we find an alternate way to validate responses against a schema.

Meanwhile, in looking more closely at how we're using this library, I stumbled on #3037852: ResourceResponseValidator::validateResponse() never gets called.

#20 + #2: Given the concerns/risks around the dependencies that justinrainbow/json-schema has, I think it may be better to consider adding this in an issue after 8.7.0-alpha1. I don't think it makes sense to stress out over these things shortly before that deadline, for a non-essential dev dependency.

I'm asking for an issue to simply filed in their github queue, not resolved. That way we've done our due dilligence to the library we're adding as a dependency -- and shared some information they may want to know.

Created: https://github.com/justinrainbow/json-schema/issues/570

Thanks @Wim Leers! That satisfies my concerns.

Up to the framework managers now. :)

I posted #3039275: phpunit-mock-objects is marked as "abandoned" when composer.lock is rebuilt for the out-of-scope issue with the lock file.

Signing off as FM on adding this library.

However, there may be cases where people have dev dependencies installed on production (I actually suspect it's quite common). For those people, we should not be executing this library's code outside of tests. Therefore, I opened #3040451: ResourceResponseValidator::validateResponse() is executed on PHP 7, even when assertions aren't active. I'm ok with that being a follow-up to #2843147: Add JSON:API to core as a stable module if it doesn't get resolved before that's committed.

@effulgentsia's #3040451: ResourceResponseValidator::validateResponse() is executed on PHP 7, even when assertions aren't active was addressed — see #3040451-3: ResourceResponseValidator::validateResponse() is executed on PHP 7, even when assertions aren't active and #3040451-4: ResourceResponseValidator::validateResponse() is executed on PHP 7, even when assertions aren't active 🙂

#11 no longer applies.

Rerolled.

Committed this as part of #2843147: Add JSON:API to core as a stable module.