API-First Drupal: file uploads — 572 comments summarized

This blog post summarizes the 572 comments spanning 5 years and 2 months to get REST file upload support in #1927648 committed. Many thanks to everyone who contributed!

From February 2013 until the end of March 2017, issue #1927648 mostly … lingered. On April 3 of 2017, damiankloip posted an initial patch for an approach he’d been working on for a while, thanks to Acquia (my employer) sponsoring his time. Exactly one year later his work is committed to Drupal core. Shaped by the input of dozens of people! Just *look at that commit message!*

Background: API-First Drupal: file uploads!.

• Little happened between February 2013 (opening of issue) and November 2015 (shipping of Drupal 8).
• Between February 2013 and April 2014, only half a dozen comments were posted, until moshe weitzman aptly said Still a gaping hole in our REST support. Come on Internets ….
• The first proof-of-concept patch followed in August 2014 by juampynr, but was still very rough. A fair amount of iteration occurred that month, between juampynr and Arla. It used base64 encoding, which means it needed 33% more bytes on the wire to transfer a file than if it were transmitted in binary rather than base64.
• Then again a period of silence. Remember that this was around the time when we were trying to get Drupal 8 to a shippable state: the #1 priority was to stabilize, fix critical bugs. Not to add missing features, no matter how important. To the best of my knowledge, the funding for those who originally worked on Drupal 8’s REST API had also dried up.
• In May 2015, another flurry of activity occurred, this time fueled by marthinal. Comment #100 was posted. Note that all patches up until this point had zero validation logic! Which of course was a massive security risk. marthinal is the first to state that this is really necessary, and does a first iteration of that.
• A few months of silence, and then again progress in September, around DrupalCon Barcelona 2015. dawehner remarked in a review on the lack of tests for the validation logic.
• In February 2016 I pointed out that I’m missing integration tests that prove the patch actually works. To which Berdir responded that we’d first need to figure out how to deal with File entity type access control!
• Meanwhile, marthinal works on the integration test coverage in 2016. And … we reached comment #200.
• In May 2016, I did a deep review, and found many problems. Quick iterations fix those problems! But then damiankloip pointed out that despite the issue being about the general File (de)serialization problem, it actually only worked for the HAL normalization. We also ended up realizing that the issue so far was about stand-alone File entity creation, even though those entities cannot be viewed stand-alone nor can they be created stand-alone through the existing Drupal UI: they can only be created to be referenced from file fields. And consequently, we have no access control logic for this yet, nor is it clear how access control should work; nor is it how validation should work! Berdir explained this well in comment 232. This lead us to explore moving parts of https://www.drupal.org/project/file_entity into core (which would be a hard blocker). The issue then went quiet again.
• In July 2016, garphy pointed out that large file uploads still were not yet supported. Some work around that happened. In September, kylebrowning stressed this again, and provided a more detailed rationale.
• Then … silence. Until damiankloip posted comment #281 on April 3, 2017. Acquia was sponsoring him to work on this issue. Damian is the maintainer of the serialization.module component and therefore of course wanted to see this issue get fixed. My employer Acquia agreed with my proposal to sponsor Damian to work on REST file upload support. Because after 280 comments, some fundamental capabilities are still absent: this was such a complex issue, with so many concerns and needs to balance, that it was nigh impossible to finish it without dedicated time.
  To get this going, I asked Damian to look at the documentation for a bunch of well-known sites to observe how they handle file uploads. I also asked him to read the entire issue. Combined, this should give him a good mental map of how to approach this.
• #281 was a PoC patch that only barely worked but did support binary (non-base64) uploads. damiankloip articulated the essential things yet to be figured out: validation and access checking. Berdir chimes in with his perspective on that in #291 … in which he basically outlines what ended up in core! Besides Berdir, dagmar, dawehner, garphy, dabito, ibustos all chimed in and influenced the patch. Berdir, damiankloip and I had a meeting about how to deal with validation, and I disagreed with with both of them. And turned out to be very wrong! More feedback is provided by the now familiar names, and the intense progress/activity continues for two months, until comment #376!
• Damian got stuck on test coverage — and since I’d written most of the REST test coverage in the preceding months, it made sense for me to pick up the baton from Damian. So I did that in July 2017, just making trivial changes that were hard to figure out. Damian then continued again, expanding test coverage and finding a core bug in the process! And so comment #400 was reached!
• At the beginning of August, the patch was looking pretty good, so I did an architectural review. For the first time, we realized that we first needed to fix the normalization of File entities before this could land. And many more edge cases need to be tested for us to be confident that there were no security vulnerabilities. blainelang did manual testing and posted super helpful feedback based on his experience. Blaine and Damian tag-teamed for a good while, then graphy chimed in again, and we entered September. Then dawehner chimed in once more, followed by tedbow.
• On September 6 2017, in comment #452 I marked the issue postponed on two other issues, stating that it otherwise looked tantalizingly close to RTBC. aheimlich found a problem nobody else had spotted yet, which Damian fixed.
• Silence while the other issues get fixed … and December 21 2017 (comment #476), it finally was unblocked! Lots of detailed reviews by tedbow, gabesullice, Berdir and myself followed, as well as rerolls to address them, until I finally RTBC‘d it … in comment #502 on February 1 2018.
• Due to the pending Drupal 8.5 release, the issue mostly sat waiting in RTBC for about two months … and then got committed on April 3 2018!!!

Damian’s first comment (preceded by many hours of research) was on April 3, 2017. Exactly one year later his work is committed to Drupal core. Shaped by the input of dozens of people! Just look at that commit message!