Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
For core-readiness, there are IMHO 3 overarching questions:
- What is the API?
- Are we confident our tests will catch BC breaks?
- Are we confident it is secure?
In other words: API surface (1), maintainability (2), security (3).
Based on these questions, I've identified the following areas (and issues within those areas) as being essential for getting JSON API into Drupal core.
JSON API spec compliance
Ensure that the JSON API module's HTTP API effectively matches the JSON API spec. (Also means fixes to comply with the spec are not BC breaks.)
Issues:
- DONE: #2917260: Validate against specific JSON Schemas if Schemata is present (validate using schemata module + OpenAPI docs are in sync)
- DONE: #2932030: Indicate supported JSON API specification version Indicate supported JSON API specification version (1.0)
Test coverage
Ensure that:
- BC breaks are detected before they are committed.
- Security vulnerabilities are proven to be absent.
- All relevant test coverage lives in the
jsonapi
module, notjsonapi_extras
Issues:
- DONE: #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only Comprehensive testing of individual resources, for the most essential entity types
- DONE: #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases Expand this to comprehensive testing of ALL entity types
- DONE: #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets
- DONE: #2953318: Comprehensive JSON API integration test coverage phase 4: collections, filtering and sorting
- DONE: #2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets
- DONE: #2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships Comprehensive testing of POST/PATCH/DELETE of relationships
- DONE: #2950486: Test coverage: custom modules' format-agnostic @FieldType and @DataType-level normalizers behavior in JSON API (test that custom modules' format-agnostic field type normalizers ARE NOT picked up)
- DONE: #2950486: Test coverage: custom modules' format-agnostic @FieldType and @DataType-level normalizers behavior in JSON API (test that custom modules' format-agnostic data type normalizers ARE picked up)
- DONE: #2950744: Move relevant test coverage from jsonapi_extras to jsonapi Move relevant test coverage from
jsonapi_extras
tojsonapi
- DONE: #2936673: Add explicit JSON API test coverage for security issues already uncovered in REST core (prove security is on-par with core REST module)
DX
- DONE: #2930231: JSON API 403 errors don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out (show reason from AccessResult::getReason() in 403 responses -> #2930028 will follow-up and test this.
API-First ecosystem
Ensuring that
- A) the same data is exposed by core REST + JSON API + GraphQL + RELAXed Web Services
- B) the pieces of data that should be normalized in the same way across all of the above, are normalized in the same way
Issues:
- DONE: #2860350: Document why JSON API only supports @DataType-level normalizers
- DONE: #2921257: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() (support non-internal computed fields)
- DONE: #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands (work-around for timestamp field type normalizer: don't wait for it to be converted to a data type normalizer, but duplicate that instead for now)
- DONE: #2929935: Remove JSON API's ScalarNormalizer: it's no longer necessary thanks to #2751325's PrimitiveDataNormalizer Assigned to: e0ipso (remove scalar normalizer in favor of core's)
- DONE: #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it (have a plan for removing JSON API's "file download URL")
- DONE: #2887313: JSON API indicates it supports POST/PATCH/DELETE of config entity types, but that's impossible (stop pretending JSON API supports config entities)
- DONE: #2937279: [META] Introduce concept of internal resource types Provide mechanism to exclude "internal" entities until a more generic option is supported by core
- DONE: #2937797: Entity UUID Converter performance
Maintainability
- DONE: #2923779: Normalizers violate NormalizerInterface (JSON API normalizers violate Symfony's NormalizerInterface)
- DONE: #2935370: Mark the JSON API serializer, normalizer and encoder services as private (mark all normalizer services as internal and private, cfr #2920536: Force all serializer encoders + normalizers services to be private)
- DONE: #2935947: Comply with code standards (comply with the core coding standards)
- DONE: #2933343: Define, document and mark scope of PHP and HTTP API
Comments
Comment #2
Wim LeersComment #3
gabesulliceComment #4
e0ipsoThe
ResourceTypeRepository
service will allow to do that. One can decorate the service and filter the resources out for that purpose. If that DX is too convoluted we could add a hook_alter (or similar), but I think it makes sense to keep that pattern we are already using in JSON API Extras.Comment #5
e0ipsoThis is a check, I don't expect much work to come out of this. Good caution nevertheless!
Comment #6
e0ipsoComment #7
e0ipsoWe'll need to come up with a reasonably complex use case for the test. We need to make sure to add that to the issue summary when one is created.
Comment #8
gabesulliceGiven that this is a temporary solution, I think the alter option is the better DX. We don't want to make developers spend a ton of time on something which they can just delete later. Let's make sure the hook is immediately marked as deprecated.
Comment #9
Wim Leers#4: hah, I made the same observation :) But it does mean this will need to become a non-internal API. So we should be very very careful with that, because we'll have to support it forever.
Which is why I'd +1 what @gabesullice proposed:
#5: Indeed, should be very little work!
#7: Indeed, details TBD. That's why I kept it very high-level.
Comment #10
e0ipsoThat's a good point. I think we have consensus on hook_alter + deprecation.
Comment #11
gabesulliceComment #12
Wim Leers#2929935: Remove JSON API's ScalarNormalizer: it's no longer necessary thanks to #2751325's PrimitiveDataNormalizer just landed! 🎉
Comment #13
Wim Leers#2932030: Indicate supported JSON API specification version just landed!
Comment #14
gabesulliceComment #15
e0ipsoI don't think we need #2923779: Normalizers violate NormalizerInterface to be added here. Any strong feelings about it?
Comment #16
Wim Leers#15: I do. Core committers will never allow a new module to be added if it violates existing interfaces. We either need to stop violating that interface or we need to thoroughly document/justify A) why we're doing it, B) why it's safe to do so.
Comment #17
Wim LeersAdded something for #2932625-7: Fix query count after query builder refactor.
Comment #18
xjm@gabesullice and @Wim Leers pointed me at this issue. Exciting! A few initial questions and comments:
This sounds like the most important piece to me for core inclusion.
Does this mean BC for the module API between the contrib module and core? Test coverage preventing BC breaks for the exposed API (i.e. the JSON API API itself, not the PHP API)? Something else?
This is impossible to prove. :) What I'd say instead is:
(If issues are identified they should be reported privately as security issues, not discussed here, since the contrib module has stable releases.)
It looks like there's some question as to whether #2923779: Normalizers violate NormalizerInterface is a required part of the roadmap or not. This would probably fall under the architectural review for the module and I expect we'd want to answer the question (whether it's "yeah we need to fix this" or "wontfix" or something in between) before the module were marked stable for core, but it doesn't need to block adding the module as an alpha module.
We can ask the framework managers about the issue when the time comes, but I think that can come after some of the other priorities and I think it's okay if it's an open question when the module is initially added to core.
Comment #19
Wim Leers+ (in chat):
That’s exactly what the concern is, and why the bar needs to be higher for the JSON API to be added to core than, say, the Settings Tray module.
All JSON API contrib module users should be able to update to JSON API in core.
It makes no sense to add an alpha-experimental module to core in which we don’t care about BC, because then we’d be screwing over the many thousands of existing JSON API users. The contrib module already cares about BC, so adding it as alpha-experimental would effectively be a step backwards. Plus, even as alpha-experimental, the iteration pace in core is much, much slower than in core.
In other words: the JSON API contrib module needs to be added to Drupal core with at least beta-level stability: https://www.drupal.org/core/experimental#beta.
Comment #20
e0ipsoFrom #19
I agree with this statement. +1.
From #18
That's a great suggestion. I'll make sure that's added to the current priorities.
Yeah, I'm feeling that we'll have to do a videochat meeting for that one. I think that the suggestion is well intentioned but I feel there are several non-obvious problems that may make this impossible (or too big of a task to be worth it). And moving forward with it will definitely break BC. That's why I'm leaning towards wontfix or do some more preliminary investigation when all priorities are knocked off.
Comment #21
Wim Leers#2933343: Define, document and mark scope of PHP and HTTP API landed!
Comment #22
Wim LeersOh and #2917260: Validate against specific JSON Schemas if Schemata is present landed 6 days ago!
This means all critical
issues are DONE! 🎉Comment #23
Wim Leers@gabesullice has opened and is working on #2935947: Comply with code standards.
Comment #24
e0ipso#2935947: Comply with code standards was merged.
Comment #25
e0ipso@xjm we met and debated about #2923779: Normalizers violate NormalizerInterface and we decided on the course of action. This should address #18.4.
PS: Meeting notes available.
Comment #26
e0ipso@xjm we also have #2936673: Add explicit JSON API test coverage for security issues already uncovered in REST core to make sure we explicitly cover the security vulnerabilities uncovered in REST core.
Is there anything lingering from your questions above that you'll like to see addressed?
Comment #27
gabesulliceWe got a ton of commits in this past week! 🎉
Comment #28
Wim LeersUpdating IS for #26.
#27: 🎉
Comment #29
Wim Leers#2932035: ResourceTypes should be internal when EntityType::isInternal is TRUE became a subset of a larger tree of issues, of which #2937279: [META] Introduce concept of internal resource types is the root. Updating IS.
Comment #30
Wim Leers#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only landed, now #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases is moving forward!
Comment #31
Wim LeersAll child issues of #2937279: [META] Introduce concept of internal resource types have been completed, so marking that done too! Go @gabesullice especially, but also @e0ipso for arriving at this solution!
Comment #32
Wim Leers#2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases got a tighter scope, and #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets was created for the second part of its original scope. Updated IS.
Comment #33
Wim Leers#2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases landed. Next: #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets and #2936673: Add explicit JSON API test coverage for security issues already uncovered in REST core.
Comment #34
Wim Leers#2936673: Add explicit JSON API test coverage for security issues already uncovered in REST core is done!
Comment #35
Wim LeersCreated #2950486: Test coverage: custom modules' format-agnostic @FieldType and @DataType-level normalizers behavior in JSON API for
Comment #36
Wim LeersIssue created: #2950744: Move relevant test coverage from jsonapi_extras to jsonapi.
Comment #37
Wim Leers#2950486: Test coverage: custom modules' format-agnostic @FieldType and @DataType-level normalizers behavior in JSON API landed!
Comment #38
e0ipso#2921257: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() is now fixed!
Comment #39
e0ipso#2940342: Cacheability metadata on an entity fields' properties is lost is also resolved.
Comment #40
e0ipsoAdded #2937797: Entity UUID Converter performance to the list.
Comment #41
e0ipsoComment #42
Wim LeersUpdating IS for #38.
#40: I don't understand why that's a must-have for this to go into core. It doesn't require architectural changes. Can you explain that a bit? :)
Comment #43
Wim Leers#2935370: Mark the JSON API serializer, normalizer and encoder services as private landed!
Comment #44
gabesullice#2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets landed!
I've added two new issues:
#2953318: Comprehensive JSON API integration test coverage phase 4: collections, filtering and sorting
#2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets
That will leave POST/PATCH/DELETE of individual resources and relationships
Comment #45
e0ipsoThere is a requirement for performance for inclusion of a contrib into core. This feels like a necessary fix in that regard. Regardless, this was merged and fixed already.
Comment #46
e0ipsoNow #2950744: Move relevant test coverage from jsonapi_extras to jsonapi is fixed. We can move all of our attention to the two remaining issues.
Comment #47
Wim LeersPOST/PATCH/DELETE of individual resources is already tested. It's indeed not yet tested for relationships. So, can you clarify?
#46: marked it as done in the IS.
#2937797: Entity UUID Converter performance also landed, marked that as done too!
Comment #48
Wim LeersI think #2955020: Spec Compliance: JSON API's profile/extention (Fancy Filters, Drupal sorting, Drupal pagination, relationship arity) needs to be explicitly communicated unfortunately belongs in here too.
Comment #49
Wim LeersThis is already done for individual resources, not yet for relationships. Checked this with @gabesullice (per my question in #47). Clarifying IS.
Comment #50
Wim Leers#49 still needed an issue. Now there is one.
Comment #51
Wim LeersYay, @gabesullice got #2953318: Comprehensive JSON API integration test coverage phase 4: collections, filtering and sorting to an RTBC state; I just committed it! Only 6 left!
Comment #52
Wim Leers#2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands is RTBC as of yesterday! It'll get committed once we wrap up the work for 1.x — to minimize divergence on patches that need to go into both branches.
Comment #53
Wim Leers#2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets just landed! 🎉 Go @gabesullice!
Comment #54
Wim Leers#2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships landed! Again, go @gabesullice!
Comment #55
Wim LeersComment #56
Wim LeersRemoved #2955020: Spec Compliance: JSON API's profile/extention (Fancy Filters, Drupal sorting, Drupal pagination, relationship arity) needs to be explicitly communicated per #2955020-50: Spec Compliance: JSON API's profile/extention (Fancy Filters, Drupal sorting, Drupal pagination, relationship arity) needs to be explicitly communicated:
Comment #57
Wim LeersWith only one remaining open (and RTBC!) issue against the 1.x branch left (#2977653: Spec Compliance: Return 204 or 200, not 201 for relationship POST requests.), we've now reached the point where the remaining work needs to happen in the 2.x branch.
Those remaining issues are:
All 3 of those can only ever happen in the 2.x branch.
Therefore moving this issue to the 2.x branch too.
Comment #58
Wim Leers#2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it landed yesterday!
Comment #59
Wim Leers#2887313: JSON API indicates it supports POST/PATCH/DELETE of config entity types, but that's impossible landed!
Comment #60
Wim LeersAnd #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands just landed, meaning this issue is DONE!
Comment #61
Evgeny_Yudkin CreditAttribution: Evgeny_Yudkin as a volunteer and at DrupalJedi commentedBut why JSONAPI duplicates rest functionality? Why JSONAPI can not work through REST plugins system?