Problem/Motivation
See #2836165: New experimental module: JSON API and #2757967: API-first initiative.
Proposed resolution
Drop in of the JSON:API module.
Remaining tasks
In no particular order,
Provide a patch. Blocked by #2931785: The path for JSON API to coreReview the code.Reviewed: see #110 and #114.Decide on stability of the module in core. Related to #2842148: The path for JSON API to "super stability"Decided: stable in 8.7.0, see #71.Provide a migration plan for users of the existing contrib module.Done: see #91.- Propose a plan for core to "dog food" this new API. (The actual implementation does not block this issue)
User interface changes
No user interface changes.
API changes
No API changes. Only additions isolated in the experimental module.
Data model changes
No data model changes.
Added dependencies
Production: None!
Dev (require-dev
):
-
justinrainbow/json-schema
:- Code quality: documented, comprehensive tests
- Maintainership of the package: recent releases (latest release was in Jan. 2019); apparently actively maintained.
- Security policies of the package: unknown
- Expected release and support cycles: undocumented; sporadic releases
- Other dependencies it'd add, if any:
"require": { "php": ">=5.3.3", "marc-mabe/php-enum": "2.3.1", "icecave/parity": "1.0.0" }
Migration from the contrib module
The ideal migration path is:
- update from
8.x-1.x
to8.x-2.x
while you're on Drupal 8.5 or Drupal 8.6; this may require some changes to JSON:API clients - update from Drupal 8.5 or 8.6 + JSON:API
8.x-2.x
to Drupal 8.7 and remove (rm -rf
) the contribjsonapi
module (this is exactly what we told sites using the contribbigpipe
module when they upgraded to Drupal 8.1)
For sites that don't want to or cannot invest in updating from JSON:API 8.x-1.x
to 8.x-2.x
: they can continue to keep the jsonapi
contrib module installed, the same 8.x-1.x
version. This overrides core's jsonapi
module. IOW: even though JSON:API is part of core, this makes Drupal ignore the core module and pick the contrib module instead. @gabesullice manually vetted this.
More information wrt JSON:API module branches and how they're supported
The JSON:API module has 2 branches in contrib: 8.x-1.x
and 8.x-2.x
. The 8.x-1.x
branch has been minimally maintained (security updates only) since June 2018, since 1.22. That branch was first maximally stabilized.
To comply with the JSON:API spec completely, we had to make certain changes that are disruptive to some JSON:API 8.x-1.x
users/clients. That's why we opened the 8.x-2.x
branch. This new branch is explicitly compatible with not only Drupal 8.6 (current stable) and Drupal 8.7 (the core minor we're targeting for including JSON:API), but also Drupal 8.5 (which currently receives only security updates). This means that JSON:API 8.x-2.x
is supported on all secure Drupal 8.x releases. Which in turn means that every JSON:API 8.x-1.x
user (which must currently use Drupal 8.5 or 8.6) can first update to 8.x-2.x
. And then update to Drupal 8.7. This gives them maximal freedom to update.
The 8.x-2.x
branch of the JSON:API contrib module will continue to receive security updates for as long as Drupal 8.6 receives security updates. And so will the JSON:API 8.x-1.x
branch.
Release notes snippet
JSON:API is now included in core. Sites currently using the 8.x-2.x branch of the JSON:API contributed module should remove the contributed module from the codebase (rm -rf modules/jsonapi
) when updating to Drupal 8.7, as the contributed module will not receive further updates aside from security fixes. Sites using the 8.x-1.x branch may require changes to API clients, but the contributed module may be left in place with Drupal 8.7 until any needed conversions are complete.
Comment | File | Size | Author |
---|---|---|---|
#185 | jsonapi-2843147-185.patch | 1.32 MB | Wim Leers |
#173 | jsonapi-2843147-173.patch | 1.31 MB | Wim Leers |
#171 | jsonapi-2843147-171.patch | 1.31 MB | Wim Leers |
#171 | create_jsonapi_patch-171.sh_.txt | 2.23 KB | Wim Leers |
#163 | jsonapi-2843147-161.patch | 1.29 MB | Wim Leers |
Comments
Comment #2
e0ipsoInitial patch.
Comment #3
e0ipsoKicking off tests.
Comment #4
e0ipsoComment #5
e0ipsoIt seems that jsonapi tests are being executed by Drupal CI and passing along with all other tests.
Comment #6
Wim LeersLeft a comment at #2836165-23: New experimental module: JSON API pointing people here.
Comment #7
e0ipsoAdded latest fixes that went in, in preparation of the core review:
This should now be ready for an exhaustive review.
Comment #9
e0ipsoTests fixed in: #2843447: Fix automated tests https://www.drupal.org/commitlog/commit/86593/9bda831174466bf6a60fca0c5b...
Comment #10
e0ipsoComment #11
dawehnerThis is just some review while going through some of the code.
Note: We don't yet use markdown files in core ... I'm fine with adding some, its much easier to read.
I guess we talk about 8.3.x as of this core patch.
Nitpick: Afaik we use
<a href="">:url</a>
instead of generating links, in help texts.For easier readability: Could we reorder the entries in this file to be ordered by priority somehow?
Nice!
Wow, its sad that we don't use the controller resolver, so we cannot easily pull values out of the request attributes.
Can we point to the place in the space where this is defined?
Note: We could use
->get("_json_api_params[$parameter_key]", NULL, TRUE)
instead of this issetIs there a reason why these exceptions are in the Error and not Exception namespace?
As somewhere else on the patch, this could use
$request->attributes->get('_route_params[_json_api_params]', NULL, TRUE)
<3 mini pagers!
... When
$field_name
doens't exist, it throws an exception, according to the documentation on\Drupal\Core\Entity\FieldableEntityInterface::get
I'm wondering whether we should check for
FieldableEntityInterface
instead, as these are the methods called hereDon't we have this service injected?
Ubernitpick: What's the reason for this dash?
Just for sanity reason I would also use $strict=TRUE as the third parameter.
Nitpick: You can also use
array_reduce($this->includes, 'array_merge', [])
here.Isn't it just sad how OOP code just doesn't really compose together. Note: You could use
array_walk($includes, [$this, 'addCacheableDependency'])
to save some lines.Wow, I had no idea that this is actually possible
I wonder whether we should use
return $has_child || $group_id == $id | $group->hasChild($id)
I thought PHP actually lazy evaluates in this case.
Can we document using
EntityInterface[]
I couldn't find any usecase for this, was this basically meant as abstract constant or so?
Wow, that's cool
Comment #12
e0ipso#2841109: Included entities that have access denied must comply with the spec was fixed in contrib.
Comment #13
e0ipsoI created #2844373: Address core feedback 2843147#comment-11872253 to address feedback in #11 before porting back here.
Comment #15
dagmarIt seems patch #12 includes much more than reported on the interdiff. Starting again from #9 I applied the patch to fix #2841109.
Comment #16
dagmarReally nice to see this issue, but now this module could be part of the core I think #2775205: [PP-2] [FEATURE] Discus how to make JSON API use text input formats for long text fields (which is blocked by #2751325: All serialized values are strings, should be integers/booleans when appropriate) becomes much more important.
My understanding is rest module and jsonapi takes different approaches to fix the problem with processed values.
So if #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata is fixed rest.module and jsonapi module is included without significant changes, fix that would require a significant amount of work.
Comment #17
Wim Leers#16:
Well, neither has an actual solution yet.
Depending on how we solve #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata, we could fix it for JSON API and REST in one go.
Also, the JSON API module will be experimental when it is added to core. So it's totally fine if that's not supported yet. We can add it later. In fact, there are lots of such missing things in core REST, yet that has also been shipping (even as "stable"). See #2794263: REST: top priorities for Drupal 8.3.x for the many things that need to be fixed in core REST.
Comment #18
e0ipsoThis patch brings the fix in #2844759: Incorrect error message on PATCH access denied.
Thanks @dagmar for pointing out the patch weirdness in #12.
Comment #19
e0ipsoThe patch above will fail because it's generated with my composer based drupal-project (which has the git root at /core). /sigh
This patch should address this.
If anyone has a good workflow to deal with this, I'm interested. Otherwise I'll write a custom script.
Comment #21
e0ipsoThe following patch fixes addresses the feedback in #11 via #2844373: Address core feedback 2843147#comment-11872253, and fixes #2844717: Validation of parameter names doesn't correctly check for backslash and control characters.
Comment #24
dagmarIt seems the patch was created outside the core/ folder.
Applied with
git apply --directory=core
and re-rolled.Comment #26
e0ipsoThe following patch catches up with what has been going on in JSON API lately.
Comment #28
mallezieSeems the latest patch, attached some git magic. (Or not related changes from an core rebase).
Comment #29
e0ipsoI definitely need to take the time to automate this process… Here's another attempt.
Comment #30
e0ipsoComment #32
Wim LeersThe fail in #29 is because we'll need to update core's
composer.json
, rather than adding our dependency incore/modules/jsonapi/composer.json
.I can write a script to automate this. Like I already said in the past: you should be able to use something like the script I used in #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.
Comment #33
e0ipsoThanks @Wim Leers. That script is not really fitting for me because I installed my local using the drupal-composer/drupal-project. That sets my core git root in
DRUPAL_ROOT/core
and notDRUPAL_ROOT
. Besides, having to merge the module's composer into core'scomposer.json
makes things slightly more difficult.It's not that it cannot be solved, it's just that it feels like a pointless chore to maintain this issue while the core committer's focus is on other pressing matters, and all the discussion is happening in the contrib module.
Comment #34
e0ipsoAlright, let's give this other one a shot!
Comment #36
Wim LeersPer https://twitter.com/webchick/status/839240958105432065, since March 7, the decision was communicated by core committers (sadly not in a very clear way, sadly not here, sadly not in the "ideas" issue queue…) that no additional experimental modules would be added in 8.4.x.
Consequently, the JSON API module must remain a contrib module for now. Which also has a bright side: we can iterate much faster in contrib. And we're already seeing a massive uptick in JSON API usage: from 240 on March 5, to 982 on April. In between those two times, the first beta release of JSON API was published, and this seems to ad adoption significantly. So, we're getting lots of contrib validation before moving it into core, which is better anyway.
The mere existence of this issue, and the publicity around it, have already helped a lot with awareness. That's in the end the key reason for wanting to add JSON API to Drupal core: to make more people aware, to lower the bar to find this module, which we believe is a better fit/offers a better DX than the REST module for many (most?) use cases.
For now, closing this issue — I will reopen this once it's possible again to add an experimental module to Drupal core.
Comment #37
Berdir@Wim That tweet *does* include API-First.. isn't json API part of that initative? This isn't something I'm specifically invested in or anything, just that the tweet as far as I see doesn't really seem to be an argument against? :)
Comment #38
Wim LeersThe tweet mentions API-First. Yes, JSON API is part of the API-first initiative (which btw still is not an official initiative… :/).
However, you need to look at the slides that that tweet links to. See slide 9. That says this for 8.4:
Keyword: contrib.
The reason API-first is mentioned, is that webchick means that core committers would like to see JSON API become fully stable in contrib first. The other reason it's mentioned is the work on the REST+Serialization+HAL modules in Drupal core:
So, while confusing, it's not actually a contradiction… But yes, the communication around all this is very confusing and very vague.
Comment #39
BerdirOk, I was too lazy to click on the link, that makes more sense :)
PS: Tried pinging you in IRC to discuss something else, pong me when you have a few minutes to discuss a maybe crazy idea :)
Comment #40
Wim Leers@e0ipso was confused by #36, much like @Berdir.
Comment #41
Wim LeersComment #42
e0ipsoThis is back on the table.
Comment #43
webchickComment #44
gabesulliceAs of #2836165-48: New experimental module: JSON API, the idea of moving JSON API into core was accepted. Which I believe means that this is no longer a Feature Request, but a plan.
I've added 3 news tasks to the issue summary:
The first is about whether it is in alpha/beta/RC or stable status. Personally, I believe that the module is at beta/RC or above. It already have a stable release in contrib (and has had one for a long time).
The second is about how sites which already have the module installed can uninstall the contrib version and install the core version. Fortunately, the JSON API module has zero-configuration and no public PHP APIs. This should be a very simple migration.
The third is, as I understand it, not a blocker for core inclusion except insofar as we have generally agreed upon plan. The JavaScript initiative already has a use case for this. They are working on a React.js implementation of the permissions page, which already makes use of the contrib JSON API module to determine which permissions are enabled for each role.
Finally, I've "unfinished" the provide a patch task because much has changed since the initial patch was provided. There are still just a small handful of ongoing issues (mostly test-related) in #2931785: The path for JSON API to core. Once those are complete, we can submit a new patch with the proposed "drop in" of the JSON API module into Drupal core (woot!)
Comment #45
gabesulliceComment #46
gabesulliceComment #47
e0ipsoI have some questions about the next steps in the process:
I will halt non-essential issues from being committed in order to ease the review process and help prioritize essential fixes. Please let me know how to best assist with this.
Comment #48
webchickI think probably the soonest we get a "needs review" patch over here, the better, generally-speaking. Transparency and visibility are important early in the development cycle. Ideally, this patch would come along with guidance on what types of review you're looking for: e.g. architectural, security, "particularly curious what people think about how this part foos the bazzes" or whatever. :) If you're worried about unnecessarily wasting peoples' time reviewing stuff that's about to be invalidated, feel free to also add whatever caveats (e.g. "this doesn't include blah and blah and blah issue yet" or "ignore all the stuff about blargle, we're ripping it out")
Usually what happens is people do the work over wherever they want (d.o sandbox, contrib module, github repo, whatever) and then update the main issue when there's something of note that's changed and worthy of another review. As it gets closer to "done" then this patch might get updated every commit.
Does that make sense?
Comment #49
e0ipsoIt makes perfect sense. Thanks @webchick!
Comment #50
Wim LeersCore committer @effulgentsia is already reviewing the JSON API module's code. I'll provide a core patch here, which will simply contain the JSON API module in its current state.
Comment #51
Wim LeersHere we go! 🎉
@effulgentsia is already doing architectural review. Architectural review also includes/implies security review. Note that
jsonapi.api.php
documents the rationale behind the architecture, and describes what the actual API surface is!We're aware that there are still a few fairly obvious gaps — that's why we have #2931785: The path for JSON API to core and have been working on addressing those. Now that the majority of those is addressed though, it makes sense to put this patch up for review.
(Note that in getting to this stage, we already did several JSON API security releases in the last few weeks: https://www.drupal.org/project/jsonapi/releases/8.x-1.10 + https://www.drupal.org/project/jsonapi/releases/8.x-1.14.)
Comment #53
Wim Leerscore/composer.json
not being updated. Fixed. (And updated shell script.)Created #2957271: [>=8.5] Fix RouteEnhancerInterface deprecation errors for this. Until that's committed, letting the shell script apply that patch.
Comment #55
Wim LeersSo, #53:
Comment #56
Wim LeersUhm, 113 passes, great, but that's not everything. Apparently that's all the functional JS tests.
Looks like something in testing is broken:
This is very strange. Retesting.
Comment #58
Wim LeersNow all tests ran. Weird.
The new failures are all also deprecation-related:
Will fix those upstream as well.
Comment #59
e0ipsoThanks for re-starting this @Wim Leers! Besides the composer thing I believe I also had to add
VERSION
to the.info.yml
in the patches from a year ago. In addition to that we'll want to clarify if core modules can have their own composer.json or those should be bubbled up to core/composer.json.Finally! I was tempted to do that in https://preview.npmjs.com/package/symfony-serializer a while ago.
Comment #60
dawehnerJust some small comments while skimming through the code.
To reduce the API surface of the core module I think it would be fine with moving this to jsonapi_extras. It is not something every decoupled site needs. This way new features can be added more easy
I just realized that this issue is closed alrady
Comment #61
Wim Leers#60: Completely agreed on both counts!
So: both things are already in progress, but thank you for confirming that these should happen before JSON API is moved into core!
Comment #62
Wim LeersTime to get this going again. Since #55, here's what happened:
$ git log --oneline --since "March 30 2018 14:21 CET" | wc -l
)That's a lot, isn't it? :)
But there's more! All of the above happened on the
8.x-1.x
branch. As described in #2952293: Branch next major: version 2, requiring Drupal core >=8.5 (and mentioned in #61), we have many reasons to start a8.x-2.x
branch. (That branch was created months ago, but we kept them identical for months.)Why wait so long? Because we wanted all >6000 JSON API users to be able to gently migrate from JSON API 1.x (on Drupal <=8.5) to JSON API 2.x (on Drupal >=8.5). And what better way to do that than to write comprehensive test coverage, and fixing all known problems that that surfaced?
That's what we've been doing the past few months! This massively reduces the risk of adding JSON API to Drupal core. We outlined a plan of must-have issues before going into Drupal core: #2931785: The path for JSON API to core — and they're all DONE as of today! Dozens of bugs have been flushed out and fixed before they ever entered core. Important: in the past 6–8 weeks we've noticed a steep drop in the number of bug reports and support requests that have been filed against the JSON API module!
After having been tasked with maturing core's REST API, and finding the less-than-great state that was in when Drupal 8 shipped, and having experienced how hard it is to improve it or even just fix bugs, this was a hard requirement for me. I hope it gives core committers the same feeling of relief as it gives me, to see that JSON API will on day one be in much better shape.
The other reason why it's in much better shape, is that the JSON API module now has no API surface other than the HTTP API! No PHP API (its sole API was dropped in the 2.x branch: #2982210: Move EntityToJsonApi service to JSON API Extras) at all, only the HTTP API as specified by http://jsonapi.org/format/.
TL;DR: JSON API in contrib today is more stable, more reliable, more feature-rich than core's REST API. And it does so while strongly complying with the JSON API spec: it's far less of a Drupalism than core's REST API.
So, with pride, and with lots of sweat (no blood and no tears fortunately), @gabesullice, @e0ipso and I present you this massively improved core patch! 🤘 💦 🎉 🍻 ❤️
EDIT: P.S.: 668K bytes of the 1.0M of bytes that this patch contains are for test coverage. That's 2/3rds!
Comment #64
Wim LeersMost of those 48 fails aren't actual fails, but deprecation errors:
We'll get that fixed :)
Moving back to
.Comment #65
e0ipsoFrom the IS:
From #62:
I think that means that we'd want to attempt to include the JSON API module with a high stability level.
So much pride! This was a long journey, that I walked (almost) alone for a couple of years. Then @Wim Leers and @gabesullice joined and carried this to the finish line. Such a beautiful collaboration!
Comment #66
Wim Leers+1
I don't see why it would not be stable right from the start: https://www.drupal.org/core/experimental#stable. Precisely because it's already battle-hardened, comprehensively integration test-covered, spec-following-rather-than-NIH-Drupalism and very few bug reports are being filed. The API surface is only the HTTP API, and that is merely following http://jsonapi.org/format/ as closely as possible. So only changes that make it comply with the spec less would be BC breaks.
Comment #67
Wim LeersResourceResponseValidatorTest
needing the Schemata module, fixed in #2983051: Allow ResourceResponseValidatorTest to run non-schemata tests when schemata is not installed.Comment #69
webchickAdding some tags.
Comment #70
Wim Leers5 failures due to deprecation errors remained, because #2982964: Add a drupalci.yml to JSON API to match Drupal core's and fix all surfaced deprecation errors only fixed those in 8.5. #2983196: Fix all deprecation errors in Drupal 8.6 just fixed those in 8.6 too.
Should be 100% green now!
Comment #71
xjm(@effulgentsia and @xjm co-authored this comment.)
It's really awesome to see the progress here on JSON API!
@xjm and @effulgentsia discussed this with other core committers (@webchick, @Dries, @larowlan, @catch) and with the JSON API module maintainers. Based on what we learned in these discussions, we've decided to target this issue for an early feature in 8.7 rather than 8.6. Therefore, we will will set it 8.7 in a few days when we branch 8.7. Reviews and comments are still welcome in the meantime, whether in this issue, or as individual issues in the jsonapi issue queue.
Feel free to stop reading this comment here, or continue reading if you want to know why it's being bumped to 8.7.
First, we want to give a huge applause for everything that everyone working on the jsonapi contrib module has done. In the last 3-4 months alone (since 8.5.0 was released and #44 was written):
Given all of the above, why not commit #70 to core now, prior to 8.6 alpha? Well,
None of the above aside from the last point are hard blockers to adding an experimental module to core. Users who prefer the stability of the 1.x module could continue to use it from contrib, thereby overriding the one in core. However, in the case of jsonapi, I think there's something odd about telling site builders to experiment with the one in core, but if they want to use it in production, to downgrade to the one in contrib. I think that people who are actually interested in using jsonapi on their sites would be better off going to the contrib project page and making an explicit 1.x or 2.x decision from there.
Meanwhile, we see what issues, if any, people run into when upgrading from 1.x to 2.x. When we're ready to commit it to core, we'll consider it at least beta stability (rather than alpha).
Once again, really fantastic work here.
Comment #72
e0ipsoChanging title based on
Comment #73
Wim LeersThose hopefully reach people that aren't following this issue!
Comment #74
pwolanin CreditAttribution: pwolanin at SciShield commentedI'm surprised this is considered viable without being able to create a new revision of a node (or have it created by default). That's a basic need for tracking authorship and compliance.
Comment #75
Wim LeersI understand it's frustrating that this isn't supported yet. But Drupal 8 core shipped with the
rest
module that was far less capable than thejsonapi
module in its current state. The issue for adding revision support to REST (#2946972: EntityResource: revisions support) has almost no followers, and in fact, it was only created 4.5 months ago. At least the JSON API module is structured in such a way that we can layer on revision support later; in core REST this is much more difficult to achieve.In other words: why block JSON API on a feature that REST hasn't had for years, whereas JSON API already has many capabilities that REST doesn't have, and has a better DX?
That being said: we will do our utmost best to get #2969022: Support content revisioning completed before JSON API is added to Drupal 8.7! My point is just that it doesn't make sense to postpone JSON API going into core until it achieves a North Star State.
Also: thank you for speaking up! ❤️🎉 The above is just my opinion. This sort of constructive criticism is EXACTLY what this issue needs! Please add more of it! Hold us accountable. Help ensure what this issue does makes sense for Drupal at large.
Comment #76
e0ipsoI fully agree on the thoughts shared by @Wim Leers in #75.
Comment #78
Wim LeersI last got this going again in #62, 8 days shy of 5 months ago.
We released JSON:API 2.0-RC2 late last night: https://www.drupal.org/project/jsonapi/releases/8.x-2.0-rc2. We believe it'll be the last RC before we release 2.0. So it's time we have a core patch here again.
As explained in https://wimleers.com/blog/state-of-jsonapi-2018-10, JSON:API 2.x:
So that brings us to the key remaining point: #71.7:
Let's get this going! (I know that @effulgentsia already started doing this, but I don't know how far he got.)
So here's a new core patch, adding JSON API 2.0-RC2 on top of core
5731bc7dab92a79d6292be65b156c91e5fa7ebea
. 676KB of the 1090KB patch is test coverage.Comment #79
Wim LeersShortly after posting it, I realized that this patch could be simpler:
drupalci.yml
is not necessaryCreated a patch for points 2 and 3 at #3015325: [ignore] support issue for the core patch. Updated the shell script to perform point 1 and apply the patch for points 2 and 3.
The patch is now a bit smaller: 1083 KB instead of 1090 KB.
Comment #81
Wim LeersHah, the wonderfully precise
hook_help()
test coverage detected that a few instances ofJSON API
were left, that should've been changed toJSON:API
. Thanks, @jhodgdon! This single failure will be fixed at #3015343: Follow-up for #3007274: s/JSON API/JSON:API/ in *.module files, once that lands, I'll update the patch here.Comment #83
Wim Leers#3015343: Follow-up for #3007274: s/JSON API/JSON:API/ in *.module files landed. Commit
b5d9167fde8db6d46cbabdcdd668d8af0e899067
. This patch should then be green :)Comment #85
Wim LeersA failure in the
PhpTransliterationTest
unit test. Huh?! I don't see how this could be caused by JSON:API. Seems like a random failure. Retesting.Comment #86
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAwesome progress!
Although this issue hasn't yet gotten the formal sign-offs for the Plan, #71 is at the very least an informal approval from both a release manager and a framework manager, and the iteration of patches since then I think squarely puts this in the Build phase of the info graphic in https://www.drupal.org/core/community-initiatives#approval-process. Therefore, I'm recategorizing this as a Feature request, but leaving the "Needs framework manager review" and "Needs release manager review" tags until those reviews are done.
Also, raising the priority to Major, since it's the top item in the API-first roadmap for 8.7.
Comment #87
e0ipsoExciting times!
Do we still need formal sign-off for this?
Comment #88
borisson_I had a couple of minutes over lunch to glance at this, I got maybe ~5% into this patch but I found some nits to pick, not sure if I should've created a jsonapi issue for this instead.
Usually
@todo
comments are linked to a drupal.org issue, so we can track when the todo needs to be removed and we know that this is still an open issue. One should be created and linked here.Also, the comment is > 80cols long.
Does this mean that this is part of the bc layer? Should this be removed instead?
Should we remove this before we get this in core?
Comment should be updated, and should link to a new issue (I think, or the one linked from here should be updated)
Comment #89
Wim LeersThanks!
Strictly speaking that's not necessary; all problems (and nits) reported here I have been fixing (and will continue to do) in the contrib module. But for the sake of focus, it would be wonderful if nits could be reported to the JSON:API issue queue, to keep the conversation in this issue about the things that make a material difference. So ideally this issue would primarily cover architectural questions.
Comment #90
effulgentsia CreditAttribution: effulgentsia at Acquia commentedGood question. Thanks for asking. The implementation still needs framework and release manager review, hence those tags.
In terms of a Plan, the things that could be considered part of the planning, per https://www.drupal.org/core/initiative-proposal-template, include:
Given the above, I think for this issue, we don't need a separate step of signing off on the plan prior to signing off on the patch. But I'm adding the "Needs issue summary update" tag for the question about contrib migration.
Comment #91
Wim Leers#90: thanks! Updated the issue summary.
Comment #92
Wim LeersWe released JSON:API 2.0-RC3 last night: https://www.drupal.org/project/jsonapi/releases/8.x-2.0-rc3.
So here's a new core patch, adding JSON API 2.0-RC3 on top of core 55f13aacb5a348c5db49042d2abca04bc126eda4.
Comment #93
Wim LeersThese classes are "forward compatibility classes" — they are being added to core in #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API. They will be removed from this patch once #2926508 lands. #2926508 was blocked on #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object, which landed yesterday! 🎉
This means #2926508 is a blocker for this issue.
#2926508 makes
@FieldType=timestamp
+@FieldType=datetime
+@FieldType=daterange
consistent, by making@DataType=timestamp
+@DataType=datetime_iso8601
consistent.Comment #94
jibranFirst of all, I'm so happy that jsonapi is going to be part of Drupal core. There are only 16 open bugs/tasks under jsonapi 2.x branch which shows the hardwork and dedication of jsonapi maintainers. 👏🏽👏🏽👏🏽👏🏽👏🏽
All the generic Normalizers should be moved to
Core
or appropriate modules before this module goes stable in Drupal core.Here are some observations:
Core doesn't have to support this. I think this can remain jsonapi 2.x branch.
Are we going to remove there before adding the module to the core? I scan the jsonapi issue queue and unable to find any related issue.
Same as above.
Core doesn't have to support this. I think this can remain jsonapi 2.x branch.
In the links section of the response, we show all the available(non internal) entities and bundles on the site. Isn't it information disclosure to show all the available entities and bundles of a site?
Comment #95
Wim LeersThanks for the review! 🙏 🎉
Yep! And many of them are actually about refactoring to make things even better in the future. :) There's only one open bug report for something very small.
We indeed worked very hard to fix all bugs and refactor the code to be as maintainable as possible.
@FieldType
normalizers. First we were only warning (hat is what you're seeing there), then we made it actually impossible/unsupported (see\Drupal\jsonapi\Serializer\Serializer::__construct()
) in JSON:API 2.x. That tag name is just an internal implementation detail at this point. It's locked down. It's not an API. Nobody can use it. We might as well rename the tag tofoobarbaz
and it'd still not be an API change. We've worked hard to ensure that all@FieldType
-level normalizer logic is not an API.How do you then add JSON:API support for your own field types, I hear you thinking? Well, field types in the end always use properties (
attributes
in JSON:API terminology — and this is also why we don't allow per-field type normalizers: to ensure they're all normalized consistently, complying with the JSON:API spec). Properties in Drupal's Entity/Field API are modeled as@DataType
plugins. And data type normalizers are supported/respected by JSON:API. The great thing is that those are format-agnostic: they work for both core REST and JSON:API. For more than a year, we've been shifting all of Drupal's API-First infrastructure in that direction — see #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem.Updated core patch and script, adding JSON API 2.0-RC3 + #3015325-8: [ignore] support issue for the core patch on top of core
b2160caa3dcf867d4850429d9ea9f693cddc04b5
.Comment #96
Wim LeersWith https://www.drupal.org/sa-contrib-2018-081 now shipped in https://www.drupal.org/project/jsonapi/releases/8.x-2.0-rc4, there now finally is no longer this only-known-by-core-committes hidden blocker. The only remaining blocker is publicly visible and already is RTBC: #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API.
RC4 will become 2.0. We will ship 2.0 in the first week of January 2019 rather than today because it'll reach more people — many people are taking time off for the holidays in the next week.
Updated core patch and script, adding JSON API 2.0-RC4 + #3015325-12: [ignore] support issue for the core patch on top of core
bda5967ecab9efc707b09a4570cc20bd8d61a0c3
.Comment #97
dwwExciting! Great work. Thanks for all your efforts, the great approach this whole initiative has been using, and the solid results!
Started reading the enormous patch, and found some extremely minor nits in core/modules/jsonapi/jsonapi.api.php:
double "is". ;)
s/he/the/
Otherwise, I love the jasonapi.api.php file and what it represents. ;) No more time today for further reviews, but this is clearly very close to RTBC.
Thanks!
-Derek
Comment #98
Wim Leers#97: On behalf of all API-First Initiative contributors: thanks! — it means a lot coming from you! 😊 I'll make sure to get those nits fixed upstream — issue created: #3021873: Two nits in jsonapi.api.php.
Comment #99
Wim LeersJSON:API 2.0 (stable!) was just released: https://www.drupal.org/project/jsonapi/releases/8.x-2.0. 🎉
So here's a new core patch, adding JSON API 2.0 on top of core a013f0538cca1edabc4a59f7d3cf09055247f194.
Comment #100
Wim LeersI noticed that one portion of the contrib migration question in #90 was not yet answered in the issue summary. Fixed that.
Comment #101
Wim LeersJSON:API 2.1 was just released: https://www.drupal.org/project/jsonapi/releases/8.x-2.1. Updated core patch using that release, on top of core
d6688ebf3c536178531ff01959057f1c4dc1785d
.The increase in patch size is because of two new features (file uploads & revisions), which come with a lot of test coverage. The file uploads functionality includes a forward port of #2940383: Unify file upload logic of REST and JSON:API.
EDIT: oh and FYI: 1700 sites are now running JSON:API 2.x, up from 400 a month ago — see https://wimleers.com/blog/state-of-jsonapi-2019-01.
Comment #102
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNeeds work for the PHP 5.6 failure (see #101's test results).
Comment #103
Wim LeersFixed in #3027501: [regression] Follow-up for #2995960 and #2992833: syntax errors in PHP 5.5 & 5.6. This core patch is JSON:API 2.1 + #3027501 (JSON:API
ea0633233a5c4b81683959dd8398741b93a41d5c
) on top of the same core commit.Queued tests against PHP 5.5, 5.6, 7.0, 7.1 and 7.2. Also queued tests against SQLite & PostgresQL.
This sadly went unnoticed because despite our repeated attempts to configure automated testing to use the PHP versions we specify, it keeps changing it. Testing against multiple core minors and/or multiple PHP versions doesn't seem to work reliably. 😔 I've reported this before. It probably isn't a high priority right now, because the DA is working on the GitLab integration.
Comment #104
gabesulliceThe two failures in #103 were caused by:
composer.lock
file (I believe)If there is a preferred way to update composer dependencies in a patch like this, please let us know!
Comment #106
gabesulliceLooks like I still hadn't gotten the composer thing right in #104. I got on a call with @effulgentsia, who helped me figure out the right way to do that (thanks!)
Comment #108
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis is great. It's adding that package dependency to
core/composer.json
correctly, but the remaining test failure is due to nothing yet adding the"drupal/jsonapi": "self.version",
line. (The #103 patch has it, but I don't know if/how that was automated.)Comment #109
Wim Leers#108: Yep, #104's update to the script lost the manual update to core's
composer.json
, which is declaring the presence of thejsonapi
module.Comment #110
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI've now spent enough time reading over the module code that I now feel comfortable removing the "Needs framework manager review" tag!
In the course of doing so, a few significant WTFs popped out at me. I've made each of them child issues of this one. You can look at the live list of them in the Child issues block in the right sidebar, but here's the current snapshot:
I or other reviewers may find more between now and RTBC, and between RTBC and commit.
As a tip for anyone else wanting to help review, I find it easiest to review the 2.x branch tip of the contrib module, with #3015325: [ignore] support issue for the core patch applied on top (which removes some legacy Drupal 8.5 and 8.6 cruft), rather than the patch on this issue. That way, as things get committed there, you're always reviewing the latest. That said, I'm also setting this to NW as there have been commits there since #109, so the patch here could use a refresh.
I truly appreciate all the great work being done by the JSON:API maintainers, reviewers, testers, issue filers, and all other contributors, and look forward to this landing in core!!!
Comment #111
e0ipso@effulgentsia thanks for the in-depth review! Hopefully we can get those issues merged soon and then have this patch updated.
SO CLOSE! 🎉
Comment #112
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAll the issues in #110 are now fixed except for #3029897: [PP-1] Do not overload Symfony's normalize() method to return objects; define a new interface for that., which will take a little while to untangle its dependencies. So, I think the next step here is to upload an up-to-date patch with where we're currently at. Either before or after that's uploaded, I'd like to add some docs that explain where the JSON:API normalizer architecture currently is, what's being attempted in #3029897: [PP-1] Do not overload Symfony's normalize() method to return objects; define a new interface for that., and what the eventual goal is in #3028080: Add CacheableNormalization for Normalizer to return Normalized value and Cacheablity. I think those docs will be both helpful to any additional people reviewing this patch, and to release managers in evaluating the "Needs release manager review" tag for this issue, from the perspective that until those issues are complete, we're not ready to commit to backwards compatibility (of the normalizer API) for the very small set of modules (like JSON:API Extras) that require adjusting the JSON:API output at any level other than a @DataType normalizer (which is the only level for which there is a supported public API). I'll work on those docs tomorrow.
As long as a release manager approves, I'm fine with committing this to core with that work still undone and without the corresponding BC promise, as long as there's discoverable issues open (which there are) for eventually getting to a supported public API for adding normalizers to any level (or otherwise customizing that level's output) for which there are known legitimate use-cases to do so.
Comment #113
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOf likely interest to release managers for when they review this: #3032787: [META] Start creating the public PHP API of the JSON:API module
Comment #114
Wim LeersNew core patch, using
jsonapi
commit8b164fbaf82257e26e47bf03b9cc4b6f95cfa3f4
. Patch size is smaller because #3015325: [ignore] support issue for the core patch is now removing even more, thanks to @effulgentsia!I'm not going to reiterate the benefits of JSON:API over REST, we had #2836165: New experimental module: JSON API for that. I want to provide some context about the depth and scope of review the JSON:API module has received.
The short version: I've been nudging it towards core-level quality for about two years. I am confident that not only is JSON:API simpler to set up, richer in features and more pleasant to use, it also is far better prepared for the future thanks to both the spec it follows and the module's code being designed for future evolution. More detail below.
I believe this should be RTBC, because:
@effulgentsia said given that extensive scrutiny that we've applied and have effectively been preparing for core for more than a year, it'd be fair for us to RTBC this patch. So … 🦙🛥😎🍻
Comment #116
effulgentsia CreditAttribution: effulgentsia at Acquia commentedYep. Specifically, I've seen the 3 primary JSON:API maintainers: @Wim Leers, @gabesullice, and @e0ipso, thoroughly reviewing each other's work in the project queue, so +1 to the RTBC. Speaking of which, next time this patch is rerolled, can we add a MAINTAINERS.txt entry to the patch? Are all 3 of you staying on as maintainers once this is in core?
Comment #117
xjmAmazing to see this RTBC! Starting a release management review:
(Edit: Moving the BC policy bit into its own section.)
BC policy
Interesting. I think we probably need to discuss/verify that we're okay with this for BC policy for this module in core.
(Nit: "backwards-compatible" with a hyphen.)
Added dependencies
We should evaluate this package against the following criteria:
This information can be added to the issue summary.
Documentation gate
The in-code API documentation is excellent overall!
Double blank lines in code fail our phpcs rules. TBD if double blank lines in docblocks do, but I assume the principle is the same.
Run-on sentence here. I think this is supposed to say:
"Plugin levels" is unclear / not a familiar concept. I think this is talking about inheritance. If so, can we talk about it more directly, i.e. they must implement normalizers that extend DataTypeSomethingSomething or that implement DataTypeSomethingSomethingInterface?
The use of "level below" and "lower level" is part of what's confusing. "Level below" implies a level down the inheritance chain (I think?) but "lower level" usually refers to code that runs earlier.
Nit: "versioning" should be lowercase.
Nit: specification-compliant.
In doing so... In doing what? Not clear from context. Would be better as "By [implementing revision support in this fashion]..." (Is that what this is saying?)
@code
and@endcode
can't be used inline like this. On api.d.o this will split the paragraph up with each code snippet getting a whole new div. It's best to use single quotes instead for inline docs like this, or backticks if single quotes are being used already in a different way.This is unclear (maybe just on account of punctuation) and has a typo. Is the following accurate?
The blank line between these will probably need to be removed.
Same question as above about "plugin levels".
Same note about
@code
blocks. For class, function/method, and variable names, we can just use them inline with no delimiter.We don't use the word "Please" in docs or UI text. It almost all cases the word can be removed. Here I would just say "Note that normalizers are..."
This could use a little work as site builder-facing documentation. How about:
"Exposes entities as a JSON:API-specification-compliant web API."
Edit: changed my recommendation above a little more; site builders don't need to care that content and configuration entities are two different things.
We'll also want to consider how to help the user choose between this and REST given just their two descriptions, but I think the above helps with that.
The third sentence here is a bit run-on and hard to read. How about:
Hmmm, usually this should link to a handbook page on Drupal.org. I don't think linking a youtube video is very accessible. We should instead create that d.o handbook page and can link or embed the video there.
I don't think we need to talk about test coverage in user-facing help documentation. (It makes sense in the developer docs in the API since that section goes on to tell developers about their own tests.)
Several of the above points from reviewing the docs gate stuff are blocking, but I'll leave this at RTBC so that it's visible to other committers for review while those docs updates are made.
Comment #118
colanThat's a very good point, @xjm. We should make it explicit outside of this context as well. Maybe on a developer docs page? I don't want to lose site of this because...
I've been using the REST API since D8 was out, but have been tracking this module as it's been progressing. And I still don't understand what value it brings over REST. The contrib project page states that it implements the spec. So then I looked at the spec, and it shed some light on what it does, but still doesn't tell my why it's better than REST. Is it? What's the difference? Why would I use one and not the other? Does REST not do the the things that this module claims to do?
As someone who does lots of API work, this isn't clear to me. So it definitely won't be clear to anyone else. Let's be sure to document this somewhere (outside of the code here).
Comment #119
e0ipso@colan this is prominently explained in the documentation of the module. It's one of the big sections on the documentation homepage. https://www.drupal.org/docs/8/modules/jsonapi
I'm not sure we can find a more prominent place for it once this lands into core.
Notice that this is the official way Drupal.org surfaces documentation. Maybe there's a better way to do it, and that's a d.o feature request that I will +1 if you end up creating an issue for it.
Comment #120
colanThanks, found it on the API Overview sub-page. When this gets moved to the core section from contrib we'll be all good:
Comment #121
xjmAh yes, so https://www.drupal.org/docs/8/modules/jsonapi is what the
hook_help()
should link instead of a youtube video. :) Edit: Or whatever the equivalent URL will be in the core handbook when we move it. Thanks @e0ipso!I think it'd also be worth adding a short section to both modules'
hook_help()
specifically linking https://www.drupal.org/docs/8/modules/jsonapi/jsonapi-vs-cores-rest-module since that's the first choice site builders are going to make.Comment #122
xjmBC policy part 2
For all the
@internal
, we should document why the thing is internal and what the consequences of using it despite the@internal
would be. For some of these, there's a specific reason already given on the class docs that can just be moved; for others, it's probably a single sentence that we just add under each that's a boiled-down version of what's in the API docs about BC. It could even link the API doc section about it, actually.Note that marking things
@internal
still isn't a totally free pass in terms of BC, because we've discovered over the course of Drupal 8's life that people can and will use internal things in their own code despite our warnings, but when sites update and trip over an internal BC break, they blame "Drupal 8 being unstable". This is why we added https://www.drupal.org/core/deprecation#internal to our BC and deprecation policy. So even though these things are marked internal, we should still plan to provide best-effort BC via the deprecation process if we need to change them in the future.This paragraph here can simply be moved to under the
@internal
for the class. Edit: And we should probably deprecate it instead of removing it when the time comes and make it simply wrap the new core API.This is what we call a "wishful thinking" deprecation (deprecating before the replacement API is committed). We don't do this in core anymore. :) The
@deprecated
should simply be removed for now and added back according to the normal process once the new core API is added.Ditto.
Comment #123
xjmLet's cover the easy core gates next...
Usability gate
No UI. N/A aside from what's already covered above for the docs gate.
Accessibility gate
No UI. N/A aside from what's already covered above for the docs gate.
Testing gate
I didn't read all the tests. But wow. Let's just say...
✅ Passed
Actually I'm not sure that's quite strong enough. How about...
✅✅✅ PASSED!!!!
Comment #124
xjmComment #125
xjmComment #126
xjmBC policy part 3
The project page for JSON API has:
So that sounds to me like a clear counterpoint to the API visibility strategy currently exposed for this module and underscores #122,1,
Comment #127
Wim LeersWow, thanks for the super fast detailed feedback! 🙏 👏 😃
Added dependencies
Two remarks before I do the work to gather that information:
this is in the
require-dev
section ofcomposer.json
, not inrequire
we could definitely live without this, but it'd mean we'd lose the ability for any site to validate generated JSON:API responses against https://jsonapi.org/'s schema. (See
\Drupal\jsonapi\EventSubscriber\ResourceResponseValidator
.)Documentation gate
All being addressed in #3033343: Address Drupal core documentation gate feedback from @xjm. Waiting for feedback from @gabesullice and @e0ipso, expecting that to land later today. One thing worth calling out here in the core issue queue:
phpcs
fails then? 🤔)BC policy
Working on that now.
Comment #128
xjmI looked into this too; some of the things in our documented standard nabout overall docblock/docgroup section ordering and formatting and the like look like they still haven't made our way into our phpcs rules. I look forward to the day that computers just handle most of this for us. :P
Ah, this is great news. I think we should still document the things I mentioned for it, but clearly highlight in the IS that it's a dev dependency only. Missed that when I was looking it over.
Comment #129
Wim LeersIndeed 🤖
🙂 Will do!
Comment #130
Wim LeersBC policy
#117:
Note that we already have such a BC policy elsewhere in Drupal core:
bigpipe.module
does the same. Both for JSON:API and BigPipe, the responses are the API not the code that generates those responses.#126: It's up to the JSON:API Extras contrib module to make changes as JSON:API's implementation changes. That's what's been happening for the past year or two.
#122.1: For most core modules the statement isn't true. And consequently I would argue that an
@internal
for JSON:API carries is more meaningful: it truly is an implementation detail.Over the course of 2018, a lot of implementation details have changed. A lot of code that was marked
@internal
was changed, renamed, moved to different parts of the codebase, or even removed altogether. The JSON:API module is designed to grow in features. Thanks to this approach, we were able to add for example revision support. If we'd hadn't explicitly used@internal
as we have, we wouldn't have been able to do that.Examples of features we probably want to add in the future: translations (#2794431: [META] Formalize translations support), partial caching (#2819335: Resource (entity) normalization should use partial caching), alternative pagination strategies (was just created), schema introspection (#2822768: [PP-1] Add information about the schema in the json:api resource), delta includes (#3006217: [upstream] Ability to include a specific delta), fetching all includes at once (#2997915: [Profile addition] Ability to fetch *all* includes), full text search (#2991729: Ability to define a "full text search" filter, e.g. using Elasticsearch), contrib module-provided hypermedia links (#3014704: Expose API to allow links handling for entities from other modules) and more. That last one is an example of an explicit PHP API.
We specifically have #3032787: [META] Start creating the public PHP API of the JSON:API module for this: to not blindly declare the current codebase as the API, but to gradually grow an intentionally designed public PHP API, based on real-world use cases and feedback. This is how pretty much every other software library, software framework and software platform does it: private APIs first, developers using those fully knowing that. I hope we can do the same for this module.
When Drupal is serving the end user application, I think it makes sense: developers just want to override "That One Thing" to get the job done. But in the case of JSON:API, Drupal is serving as the API for the end user application. The changes/overrides then tend to happen inside the end user application that consumes JSON:API.
Comment #131
Wim LeersYay, #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API landed yesterday, which was one of the two blockers to landing JSON:API in core! Updated #3015325-32: [ignore] support issue for the core patch to remove the forward compatibility layer, and now the core patch is again a bit smaller :)
Also, all docs feedback was addressed in #3033343-10: Address Drupal core documentation gate feedback from @xjm, and I credited that commit to you, @xjm, since I was able to simply copy/paste many of your suggestions, so you'd already done much of the work — thanks! :)
Comment #132
Wim LeersForgot to upload the updated script that rolled #131's patch.
Comment #133
e0ipsoI'm +1 on this. I really hope @Wim Leers and @gabesullice are in as well, we're a kick ass team together! (of course I already know their answers, but I'll let them say 😛)
Comment #134
gabesulliceI'm in.
@e0ipso 🤜💥🤛 /me 🤜 @Wim Leers?
Comment #135
Wim LeersOf course 🤜💥🤛 — also: 🕺🦙
Comment #137
catchOn the bc policy, I think it's sensible but one question:
On building to the specification rather than the module though, how does that work for people in practice if they run up against a bug - should they look for things how they should be, then if they don't find what then fall back to the actual behaviour the module is doing pre-fix? If that's the case it might be worth expanding the comment to include advice - i.e. if you code to the spec but the module has a bug that breaks this, you're a bit stuck.
I personally don't mind marking everything @internal and expanding it later explicitly, this is primarily not a module people will integrate with (like bigpipe) and it doesn't store any data/define any entities either. It's slightly different from marking things as @internal that are obvious extension points 'just in case'.
Comment #138
Wim Leershttps://www.drupal.org/sa-core-2019-003 caused the release of https://www.drupal.org/project/jsonapi/releases/8.x-2.3, so we now need a reroll. JSON:API
11f4b19fbe5768eeb4a02c08e42b43cbf918ab07
on top of cored4fa72e7cf2cee035100a5c62c762d9af161d409
.#137: 👌👍 Regarding spec compliance bug fixes: discussed this with catch in chat. Improved documentation in #3035085: Clarify client expectations wrt server spec compliance fixes, with @catch's +1.
The requested
MAINTAINERS.txt
additions have been added to the shell script.Comment #139
gabesulliceUbernit: Let's go by the order that each of us got commit permissions to the contrib module, not alphabetically.
Comment #141
Wim Leers#139: somebody on Slack pointed out that the goal is to eventually list these alphabetically, so let's just keep it like this for now.
#138 was green.
Until #2244513: Move the unmanaged file APIs to the file_system service (file.inc) landed two days ago, which introduced deprecations in 8.7.0 that cannot be fixed in JSON:API itself (since it needs to continue to work with Drupal 8.5.x and 8.6.x), which we fixed in #3035666: Drupal core compatibility: file_* functions deprecated in Drupal >= 8.7. Also, if #2940383: Unify file upload logic of REST and JSON:API had already landed (the last blocker for this issue), this reroll wouldn't have been necessary.
Rerolled using
jsonapi
3e062ac4cf674ef91238c0545ecad112f62c1e81
plus Drupal corece4034eef635036a57ea14e0b9d7593450a59c7b
with #3015325-39: [ignore] support issue for the core patch.Comment #142
gabesulliceAdded a brief section on dependencies to the IS.
justinrainbow/json-schema
is the only dependency and it's in thecomposer.json
dev
section.Comment #143
Wim Leers#142 updated the issue summary as requested.
I also just updated other parts of the issue summary. Removing tag.
Comment #144
Wim Leers#2940383 was the last blocker. As of #2940383-60: Unify file upload logic of REST and JSON:API, it was decided that it'd be better to not provide an "file field uploader" service that both core REST and JSON:API use, but to instead work on such a service that works for not only REST and JSON:API, but also UI file uploads. In other words: the decision is to wait until after this lands so we can "get it right the first time", thanks to the rule of three.
Therefore this issue now has no more blockers!
As far as I can tell, the only remaining bit of feedback that was not yet addressed here, is the backwards compatibility policy feedback from @xjm. That was addressed last night in #3033359: Address Drupal core BC policy gate feedback from @xjm.
So: attached is a core patch with all feedback addressed 🥳
Core patch:
jsonapi
4ed6779ca3034cd790585c565471f38c005fd354
plus Drupal coref5cd28b7b877e4093eb83b8472a49e368019549e
with #3015325-43: [ignore] support issue for the core patch.Comment #145
dwwRe: #144: "with all feedback addressed"
Almost. ;) If JSON:API is not using #2940383: Unify file upload logic of REST and JSON:API there's unaddressed feedback in the JSON:API - specific version of the file uploader service (*sigh*). E.g.
jsonapi/src/ForwardCompatibility/FileFieldUploader.php
is still callingbasename()
directly, perhaps leaking FDs in various error conditions, etc. I asked in Slack about this but you might have missed it. ;) Should I post the review here or in the JSON:API queue?Comment #146
Wim LeersAh! Thanks for being so diligent, @dww :) And that is of course exactly why #2940383 exists: to prevent parallel implementations of the same things. This nicely illustrates that :)
It'd be great if you could file an issue in the JSON:API issue queue for that. If you do it here (which is also fine), we'll just have to convert your comment into an issue.
Comment #147
dwwOkay then, added #3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource as a major "blocker" child. See you there. ;)
Cheers,
-Derek
p.s. Does that mean this should be pp-1 and postponed? I don't want to mess with the status since everyone's worked so hard to get to RTBC already...
Comment #148
gabesulliceI've created #3036210: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses at the request of @effulgentsia and @xjm to further delve into #142.
Comment #149
dwwUpdate from my files-related deep-dive:
#3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource is now committed.
#3036251: JSON:API TemporaryJsonapiFileFieldUploader::streamUploadData() can call fclose(FALSE) is RTBC and awaiting a commit of the corresponding bug report for REST in core (also RTBC): #3036197: REST FileUploadResource::streamUploadData() can call fclose(FALSE)
So I'm signing off on my file-related concerns for this (at least until we can revisit via #2940383: Unify file upload logic of REST and JSON:API).
s/imaginary-postponed/RTBC/ ;)Thanks!
-Derek
Edit: p.s. although we will need another patch re-rolled that includes the fixes.
Comment #150
Wim LeersBoth #3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource and #3036251: JSON:API TemporaryJsonapiFileFieldUploader::streamUploadData() can call fclose(FALSE) have landed. All of @dww's file-related deep-dive (thanks! 🙏) have been addressed. Since then, #3000057: Deprecate drupal_set_time_limit() and file_upload_max_size() and move to Environment component added another file-related deprecation to 8.7, so JSON:API had to be updated accordingly, to not trigger the deprecation error (done in #3036772: Drupal core compatibility: file_upload_max_size() deprecated in Drupal >= 8.7).
Fresh patch, JSON:API
d7f976a70fc7a7ea964df1b9474e5a0e98f6adea
+ #3015325-46: [ignore] support issue for the core patch + Drupal31fbee706ea854f7732773bed5f002a56cf70935
.Meanwhile, #3036210: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses has a patch ready.
Comment #152
xjmI reviewed the docs gate fixes. Looks good to me! Just one thing I think we should improve further:
I would make this a little more clear to the uninitiated, maybe something like:
The <a href="">JSON:API</a> and <a href="">RESTful Web Services</a> modules serve similar purposes. <a href="">Read the comparison of the RESTFul Web Services and JSON:API modules</a> to determine the best choice for your site.
And then let's add that same paragraph to rest's
hook_help()
too, in this patch.Comment #153
xjmMaintenance and technical debt
https://www.drupal.org/project/issues/search/jsonapi?project_issue_follo...
There are 67 open issues, including two criticals and 12 majors. I haven't read through them all yet, just skimmed titles. There are three issues that concern me based on their titles:
The WSOD should ideally be fixed before we include this in core. Looks like the issue is on track for a fix.
The revision and translation support issues worry me more. Generally, revision and translation support are things we've made stable blockers for other core features. Can I get a quick TLDR of where revision and translation support is at for the core patch?
Security
As of this writing, all known security vulnerabilities with the module have been resolved. However, the current "no-config" design does result in increased attack surface, including an increased attack surface compared to core REST. There is a blocking security hardening discussion still underway for this:
#3035979: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities
(Interested parties should chime in over there. We'll continue to review the discussion in that issue.)
Performance gate
Since this is new code that doesn't touch anything outside the added module, presumably this won't regress any existing functionality. However, I haven't reviewed this patch for any of the specifics of https://www.drupal.org/core/gates#performance and will leave this to the framework managers.
Dependencies continued
We are reviewing this in #3036210: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses and should consider that issue a blocker for this one.
Other stuff
Regarding #130, #137, and the added docs for
@internal
: I'm comfortable with all that so long as it's understood that we will still ask for core's deprecation process to be used even for this internal code, as per: https://www.drupal.org/core/deprecation#internalUh what?
I'm assuming this is some side effect of rebuilding
composer.lock
and totally unrelated to JSON:API? Maybe having to do with our use of an old version of PHPUnit? But would be good to know why that's happening here and what impact this would have on Composer builds if any.As per my previous review in #122, we should only add the
@deprecated
after the replacements have been added to core. This can be converted to an@todo
instead.Comment #154
Wim Leers#152: ✅— done in #3037682: Address additional Drupal core documentation gate feedback from @xjm and uploaded patch for
rest.module
to #3015325-47: [ignore] support issue for the core patch and updated the script to apply it.#153:
Maintenance and technical debt
The second issue was marked as a critical bug by an (understandably frustrated!) Drupal user. It wasn't actually a critical bug. We were giving the issue some time. I've now closed it and updated the issue in another contrib module issue whose patch was causing this, to propose a patch change that would avoid the problem :)
The first and third are about translation and revision support. I've updated their issue summaries. For your convenience, here is a summary for each:
So: revision support is present insofar as possible (read-only, only certain entity types, due to the Entity Revision API not being fully fleshed out, the Workflow Initiative ran into this as well), translation support is present thanks to "entity" route parameter upcasting (read-only, edge cases not handled in a way that's great for a web service).
I want to call out that of those 65 (down from 67) open issues, 31 are feature requests. 7 are plans. 25 are tasks (usually about refactoring something). Only 2 are bugs. In other words: the functionality that we do have is reliable, the bulk of the issues are about adding more features :)
Also, 2 criticals and 12 majors sounds relatively scary, but … per the above, it's really just one critical (for adding JSON:API 1.1 support when that spec is finalized, we've already vetted that we're indeed fully compliant, the remaining work is about communicating which profiles on top of the base specification our implementation provides). I triaged the issue queue again and we're now down to 10 majors. There is only one major bug.
Performance gate
I'll just point out that I've worked on lots of issues making the REST module faster, and all of those have also been applied to JSON:API. JSON:API just like REST takes advantage of Dynamic Page Cache and Page Cache (and has thorough test coverage). JSON:API's performance is much better than when REST's was when Drupal 8.0.0 shipped.
The key difference wrt performance between REST and JSON:API is that JSON:API supports collections and filtering of collections. This is mapped directly to Entity Field queries. So JSON:API benefits of years of maturing and optimizing that existing API.
Other stuff
composer.lock
will end up adding this. It's because https://github.com/sebastianbergmann/phpunit-mock-objects/ has been abandoned, since it was merged into PHPUnit itself. We see this appear because we're still using an older version ofphpunit
. 100% unrelated to this issue :)Comment #155
Wim LeersPair-written by @gabesullice and I.
#153.1:
It seems like there's a contradiction between https://www.drupal.org/core/d8-bc-policy#bc and https://www.drupal.org/core/deprecation#internal. In the former, it says and in the latter it says . I'm trying to reconcile these two statements and I think what it's saying when/where there's a real API, then that needs to be carefully deprecated.
However, the JSON:API module does not have any "internal APIs". There are no hooks, no interfaces, no plugin types, no service tags that JSON:API uses to achieve its functionality.
Perhaps there's a misunderstanding about what code is and isn't an internal API?
Comment #156
dwwRe: #153.2:
@see #3033761: Package phpunit/phpunit-mock-objects is abandoned which is duplicate with:
#2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5
Comment #157
xjmThanks @Wim Leers!
I included tasks and feature requests in the issue list because #2794431: [META] Formalize translations support for example was filed as a feature request. It's since been corrected, but I always look at all the issues myself to look for things I consider essential features. :) The bullet was just informational about how I'm doing my ongoing review of the technical debt, not positive or negative. The work to date shows that the module is really well maintained!
Regarding #155: The internal API policy was written in 2014, and those words you highlight were added by someone who is not a part of the community anymore. The deprecation policy was written in 2017 and updated in both 2017 and 2018 to take into account our experiences with what happened with actual minor releases from 2016 on. So lots of the internal API policy needs to be updated; AFAIK it doesn't even mention deprecation (or if it does, it does so only barely).
The deprecation practice for internal APIs is actively enforced, within reason. Sometimes providing BC and deprecation actually is dangerous or makes the DX really bad, and in those cases we justify the break, write a CR, and consider it for release notes. So it's best to start by simply asking "How can we provide BC?" and then trying that with best effort. Exceptions are discussed on a case-by-case basis on issues where they come up.
Comment #158
Wim Leers@effulgentsia, @xjm, @gabesullice and I had a call about #155 last night, after she wrote #157. It boils down to @xjm being convinced that it will not be a blocker to progress, but @gabesullice and I not being convinced of that. @xjm has since improved https://www.drupal.org/core/d8-bc-policy (thanks! 👏🙏), which certainly reduces our concerns.
Just now, I had a chat with @catch. He pointed to #3029197: [symfony 5] Class implements "Symfony\Component\DependencyInjection\ResettableContainerInterface" that is deprecated since Symfony 4.2, use "Symfony\Contracts\Service\ResetInterface" instead as an example that even dropping an interface entirely is possible. He pointed out that changing something about the container obviously is less obscure than JSON:API internals 🙂
I also had a chat with @e0ipso to catch him up.
@e0ipso, @gabesullice and I would have preferred for it to be objective and crystal clear for JSON:API, but it's going to remain a subjective decision. Given the extra context we've been given, we're okay with moving forward as-is. I consider #155 / #153.1 addressed.
I wrote this alone, but I believe I've captured @e0ipso's and @gabesullice's opinions too.
Comment #160
tedbowI am not seeing the actual test fail in #159 and have this same thing happen in another issue today so I am assuming this is drupalci hiccup.
Comment #163
Wim Leers#160: thanks :) That was indeed a DrupalCI problem.
We've addressed the remaining concerns wrt:
Further:
Fresh patch, JSON:API
b175c3a9fd5cebcb7849de7559f56a73f756d8bc
+ #3015325-48: [ignore] support issue for the core patch (I'm posting this without waiting for tests since DrupalCI is apparently again broken: still queuing after 45 mins) + Drupale716af79b9335f193baeded8d7c1f40d9dc9490b
.Comment #164
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLast I heard, the release managers are ok with giving this an extension until beta, given that it's only held up on #3035979: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities.
Comment #165
webchickI was asked to weigh in from a product manager POV on revisions and translations support.
Since we do not have "100% full compatibility with each and every one of the ever-ballooning matrix of core features" as one of the Drupal core gates, I can't see holding this feature up on that, especially since REST has the same limitations. As long as we are not introducing new bugs, particularly those that would be classified as "critical" such as data loss/security bugs, I'm in favour of an iterative approach to adding features to core, and not denying 100% of users access to something awesome because it doesn't yet meet 20% of users' needs, for example.
Nevertheless, Integration issues such as these, where feature A and feature B come into contact and there's friction, tend to be some of the most painful, confusing, and frustrating for our users, so it is very good to take them seriously. I gave https://www.drupal.org/docs/8/modules/jsonapi/revisions and https://www.drupal.org/docs/8/modules/jsonapi/translations a once-over. I'm impressed with the straight-forwardness of the docs to help developers understand what limitations exist in these areas, as well as the general plan for resolving them in the future.
So +1 from me on the product side for these (well-documented) limitations not standing in the way of this feature.
Comment #167
xjmThe test fail is in
Drupal\Tests\jsonapi\Functional\MenuLinkContentTest
so I think it's legit. https://www.drupal.org/pift-ci-job/1226063Comment #168
plachLikely caused by #2880152: Convert custom menu links to be revisionable.
Comment #169
volegerand #2880149: Convert taxonomy terms to be revisionable
Comment #170
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI provided a fix for those test failures in #3015325-49: [ignore] support issue for the core patch, but I'll let Wim post a new patch here because it seems to be quite an involved process :)
Comment #171
Wim Leers#165: 👍
#170: thanks again for that, @amateescu, and thanks @xjm, @plach and @voleger — all of you were credited in #3039235: 8.7.x: update anything needed now that taxonomy terms are revisionable :)
Since #163, the following 3 issues were fixed/committed:
Term
andMenuLinkContent
's data model additionsFresh patch, JSON:API
39c29e2b41a05bc9dde06e66f7e2978f433b010d
+ #3015325-53: [ignore] support issue for the core patch + Drupal9f6c7c1e547b9a21335e5800d0a68d1efa079cf7
.Comment #173
Wim LeersApparently DrupalCI for contrib modules will gladly ignore tests that are misplaced, but not for Drupal core, which is how it went unnoticed that I'd put the new test that #3039568: Add a read-only mode to JSON:API added in the correct place but assigned it a slightly incorrect namespace. 😭
Fixed that in #3040186: Follow-up for #3039568: ReadOnlyModeUpdateTest's namespace is defined incorrectly, causing the test not to run, and that's the only change compared to #171.
Fresh patch, JSON:API
d5abca9f600000e142516ae24e0058bdfcb9111d
+ #3015325-53: [ignore] support issue for the core patch + Drupal9f6c7c1e547b9a21335e5800d0a68d1efa079cf7
.Comment #174
Wim LeersI made a stupid mistake in #91: I wrote "uninstall" but I should have written "remove".
Also wrote a release notes snippet, catering to both possible update path scenarios: update from the contrib JSON:API module at version
8.x-2.x
or from version8.x-1.x
.Comment #175
Wim Leers@xjm pointed out that for some sites, it may be preferable to remove the contrib module after updating to Drupal 8.7.
Comment #176
xjmSimplifying the release note a bit after discussion with @Wim Leers.
Comment #177
xjmComment #178
xjmComment #179
xjmAlright, consider this release-manager-reviewed!
I checked over the queue one more time. The notewothy remaining issues are:
Beyond that, all my concerns have been addressed. I also checked with @catch and while he hasn't done a full review of all the everything himself, he's comfortable with this being untagged.
I did notice that we don't have a CR attached here yet, so let's add that.
Great work!
Comment #180
e0ipso@xjm is it worth mentioning in the release notes that sites with 8.x-2.x will work without any additional modifications upon contrib deletion? Maybe:
Comment #181
Wim Leers#179:
I'm already working on it!
P.S.: in that particular issue, we're dealing with including eighty relationships, which are in total referencing about forty different entity types/bundles. It's about optimizing an extreme case.
Draft change record created: https://www.drupal.org/node/3041438 (got inspiration from https://www.drupal.org/node/2924128 and https://www.drupal.org/node/2968491 — the change records for Layout Builder & Workspaces).
Comment #182
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks for the CR. I plan to commit this once we're out of commit freeze unless there's commit-blocking feedback between now and then.
Comment #183
e0ipsoWoo! @effulgentsia I'll be drumrolling until the commit.
Comment #184
webchickCommit freeze lifts Thursday, so make sure you've got gloves/mittens to go along with your drumroll. ;)
YAY! Thanks, @effulgentsia!
Comment #185
Wim LeersSince #173/#179, two bugs were fixed, and both come with an explicit regression test :). I've also worked quite a bit on #3039730: Include paths are resolved for every resource in a resource collection, instead of once per unique resource type (which was called out by @xjm in #179.3): it now has a clearer title, tighter scope and simpler patch. It's now blocked on feedback from the person who reported it, to test it in their pretty extreme scenario. It's really cool to see JSON:API being used in such scenarios: it helps discover opportunities for improvement that may otherwise have gone unnoticed!
Attached patch = JSON:API
fb7bf82c7acec2a2576bc01c2b51374593155d02
+ #3015325-53: [ignore] support issue for the core patch + Drupald0c3e6432099e571b5c3f8de8d8d866cb803dec1
.🥁
Comment #186
Wim LeersMight I also suggest crediting everyone in https://www.drupal.org/node/2723491/committers? They all contributed to this patch!
Comment #187
effulgentsia CreditAttribution: effulgentsia at Acquia commented+1 to crediting everyone in https://www.drupal.org/node/2723491/committers. First, checking the checkboxes for the people already on this issue. Next, I'll add everyone else.
Comment #196
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #204
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #212
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #218
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #227
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #231
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #233
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe 8.8.x branch is now unfrozen, so pushed to there!!!!
The 8.7.x branch is still frozen, so leaving at RTBC for cherry picking to it tomorrow.
Brilliant work, everyone! Sorry, that's a massive understatement, but I don't have enough superlatives to write into this comment to give it proper justice.
I'm super excited to see a whole new era of decoupled sites, slick apps, and everything else that this will enable!
Comment #234
xjmComment #235
Wim Leers🥳
It's kinda hard to believe this is happening!
Comment #237
xjmBackported to 8.7.x! Congratulations everyone and thanks for all your work. 🎉🌎🍻💫
Comment #238
Gábor Hojtsy🎉🎉🎉 Congrats and thanks all! 🎉🎉🎉