Update Schemata to be compatible with JSON:API 2.4

Can you please share the version of the Schemata module?

I have also encountered the same problem. I'm using the latest version of the json API(2.4) and schemata alpha5 version
Here is the combination of the version of the following modules.
Drupal Core: 8.6.13
JSON:API: 8.x-2.4
JSON:API Extras: 8.x-3.5
Simple OAuth: 8.x-3.14
Consumers: 8.x-1.9
OpenAPI: 8.x-1.0-beta2
Schemata: 8.x-1.0-alpha5
OpenAPI UI: 8.x-1.0-rc1
ReDoc for OpenAPI UI: 8.x-1.0-rc2
Swagger UI for OpenAPI UI: 8.x-1.0-rc3

I have downgraded my JSON API and jsonapi_extras to 2.3 and 3.4 worked for me.
composer require drupal/jsonapi:2.3.0 drupal/jsonapi_extras:3.4.0

I am using Contenta as well and am experiencing the same problem. Is this still an open issue?

Seems to be, I am also seeing it. The proposed patch does not seem to work.

#5 seem to be the way to go at the moment. It works for me.

This should fix the original bug report. However, it does not make Schemata completely compatible with JSON:API 2.4 because the response now fails validation. The schema file needs to be updated as it was in #3037852: ResourceResponseValidator::validateResponse() never gets called.

Whoops, attached.

Thanks Gabe. Code looks good.

Setting back to needs work to fix the tests. I actually prefer to have a broken HEAD with red tests than a broken HEAD with green tests.

+++ b/schemata.services.yml
@@ -1,6 +1,9 @@
+    decorates: serializer.encoder.jsonapi

Can we also specify the dependency on the drupal:jsonapi
Also, as this module is only available in the 8.7. It would be great if we'll update the info.yml to specify it clearly.

@e0ipso,
Thanks for pushing this.
What would be your recommendation for the 8.6? Should we stick with the older version OR will schemata module provide support for both 8.6 and 8.7 versions?
Note: We're using it on a client project in 8.6 and don't have any plans to upgrade to 8.7 due to the given commitment of deliverables.

This issue should address the schema changes #3053272: Update JSON:API Schema generation. However, it won't fix the failing tests here.

@naveenvalecha this needs to work with both drupal:jsonapi and jsonapi:jsonapi.

This is a highly non-ideal fix, but let's see if it fixes the underlying issue.

This commit didn't correctly fix this patch and SHOULD be reverted.

I don't think changing the service was the correct action here. This service doesn't deal specifically with api_json content; it also deals with rest serialization. The `api_json` version will never be the correct implementation. If anything, we could decorate the core service `serializer.encoder.json` which jsonapi extends in order to implement its service.

However its still unclear to me why we need to decorate the service to begin with. The orignal issue and patch were directly related to how jsonapi is interacting with schemata, in a way that it shouldn't be. That SHOULD be fixed in jsonapi, not here. If that's as the result of some mechanic that's being abused by jsonapi, we can find a better way to go that route, but we shouldn't be committing and tagging releases which break this module.

I hear you @richgerdes. However, I think the use of schemata is leans very heavily towards JSON:API. And for this use case it was known to be broken.

I think this fixes it for the majority of use cases of Schemata, which is an improvement. I think that leaving a critical issue stagnant (which happens often Schemata, because we are all volunteers with limited availability) is worse than a "broken fix".

All that being said, I think we agree on the next steps:

1. Figure out the correct fix.
2. Undo the changes here and apply the correct fix.

So I think you are incorrect here. Schemata itself doesn't have any more code for jsonapi then it does for rest, It actually has less. There are 3 sub components within schemata_json_schema, one for json, one for hal+json, and one for jsonapi. As a result, by making the patch we did, we broke 2 of 3 functions of this module for sites not already using jsonapi.

Now, as far as the user base and the number of users actively using schemata with jsonapi vs rest, I can't give specifics, but since schemata doesn't provide, at the moment, anything major to jsonapi I would expect that its a fairly low use rate, and breaking the module by adding a jsonapi dependency creates more bad then good.

Anyways, since the patch has been committed, we need to just be more careful in the future when committing that we pay attention to our users as much as what we need.

The next steps here is to get this module stable. I think my recommendation is to remove the dependency on jsonapi. I think the best solution is to effectively revert the change made in the patch and add a new service, which wraps serializer.encoder.jsonapi if jsonapi is enabled. I do like @gabesullice approch of using the inner serializer, so I will actually modify his logic here to provide a series of services. Leveraging the existing one for json, and conditionally adding ones for jsonapi and hal+json if the respective modules are enabled. I think long term, this needs to be converted into a plugin system going forward, so that this can be done in a generic way. I believe #2914963: Explicit declaration of Schema Types lays the ground work for that design, but this needs to be extended to include plugins for sourcing the schema data as well.

I will work on a new patch which handles the above new services.

The attached patch ports the change made above to the other supported formats (hal and plain json).

I'm still not 100% sold that this is the right approach since ultimately this implementation says we are encoding schema_json as the inner type (say api_json) which isn't actually the format we expect it to be.

I think there are two major considerations here. That being said, explicitly defining the supported inner schema types is ideal. I think we need to consider why the sub type is required here. I actually don't believe it is. As such, we should just directly extend Drupal\serialization\Encoder\JsonEncoder the way jsonapi does. The way this is actually being done (with decoration) seems like a huge hack, since we actually have two instances of the decorated services code, which isn't required.

I'm going to revisit this patch tomorrow, in order to verify the information above.

I think its important to understand why the issue arose in the first place, which is the result of schema validation happening within jsonapi, right? As such, I think it would make sense to move that logic into schemata, and do the validation via call to the schema factory service, passing an data object, then running the relevant dependency (justinrainbow/json-schema) into schemata instead of jsonapi. I discussed doing this already with @gabesullice previously. Moving the validation logic is simple since its already its own handler. Only part that may take effort, is porting this validation subscriber over to rest since we want to have a consistent api.

Marking as needs review so test will run.

Thanks for taking the time to write this one! I agree we have to fix this properly. I'm not worries about the PHP5.6 errors as long as they are PHP related (and not other combination of things).

+++ b/tests/src/Functional/SchemataBrowserTestBase.php
@@ -50,6 +50,7 @@ abstract class SchemataBrowserTestBase extends BrowserTestBase {
+    'jsonapi',

Do we really need to keep this here? Are we testing anything JSON:API specific?

We shouldn't need an explicit dependency here. It was added in an attempt to eliminate the errors. Attached patch removes the extra requirement.

One tests patch on this patch, I'm going to commit this, so dev is passing tests and we can move onto the other patches in the queue.

#27 has been committed and pushed.

Thanks for fixing this properly @richgerdes! 🙏🏽