Adding Bundle Subclasses to Drupal Core 9.3.0 (Part 3)

In Part 1 of this blog series, we explored the concepts of entities, bundles, entity classes, and the new feature of bundle subclasses now available in Drupal Core 9.3.x. In Part 2, we looked at some specific code samples to see some of the ways to use this feature to make your life better. Now we turn to the story of what it took to get this feature included in Drupal Core: using a Gitlab merge request (MR) instead of patches, what it takes to get something like this committed, how and why to contribute “upstream,” and more.

From a Working Patch to Being Part of Drupal Core

For better and for worse, it can be quite a challenge to get an issue from, “That patch does what we need,” to being committed to Drupal Core and released. This is particularly true for a big feature or bug fix with a lot of potential impacts and ramifications. There are a number of “gates” that every issue must pass through to be considered for official inclusion: performance, usability, accessibility, automated tests, and more. Each change needs to be carefully reviewed to ensure it satisfies all the requirements, that the code makes sense, that the tests cover everything they should cover, that the documentation is clear, and so on.

I knew we’d be very happy using this feature. I also knew we’d be sad to have to keep rerolling the patch to apply and work with future versions of core. Otherwise, we were trading one kind of technical debt (sprawled out procedural code) for another (patches to core we’d have to keep supporting). I made the case to the rest of the TEN7 team that it was in all of our interests for me to dive in and try to get this thing committed. Since I work for a shop that truly understands the value of open source contributions, it was an easy case to make. I jumped in and did all I could to move this thing along and get it to the point that a core maintainer could find no fault in it and actually commit it. This included:

Making sense of the open questions
Finding answers to the open questions
Fixing the behavior
Updating the tests to validate the behavior
Soliciting reviews and feedback, cultivating consensus
Beautifying the code and documentation
Updating the change record
Fixing the regression we didn’t think of at first

Reflections On Using a Gitlab Merge Request (MR)

By Aug 24, 2021, when I first got involved in #2570593, the issue had been open almost six years, had 50 files attached (mostly patch and interdiff), and already had two different merge requests open in Gitlab: 200 and 994. The very first step was to figure out how to even begin to move the issue forward, since I had to assess the state of two different Git branches + MRs, plus the latest versions of patches uploaded to the issue.

994 was only opened because 200 at the time was pointing to the wrong core branch. Only core committers or the original author of a Gitlab MR can change the target branch. With the Drop Always Moving, and new core branches opening up every ~6 months, MRs often need to switch branches over the course of their life. As it was, no one with the power to move 200 was active, so someone opened 994 from the right branch and rebased the work into there. However, the code in MR 200 was at the time full of useful review threads, most of which were unresolved. We’d lose all the review history if we stopped using 200. So I got everyone to agree to close 994, and @larowlan switched 200 from 9.2.x to the 9.3.x branch. I could then rebase the issue fork from the latest 9.3.x branch of core and get us back to a working, viable merge. Progress! 🎉

Lesson 1: It’d be better if it was easier for the community to change the target branch for an existing MR, so you don’t lose review history by opening a new MR.

The next step was to systematically review all the MR threads and see what code needed fixing. Many of the threads had already been addressed. Sadly, even though I’m a core subsystem maintainer, I have no way to resolve open threads on an MR. Again, only core committers (about eight people) and the author of the MR can do that. I find the review threads really helpful, but it’s painful that we can’t collectively close the ones that have been addressed. I’d much rather not ping core committers for this sort of thing. We should be able to tidy up the MR in advance before they even have to spend their time reviewing it. Ideally, there’d be no open threads for them to wade through, and they could start with a clean slate while reviewing the diff itself.

I went through all the threads, commented on the ones that could be closed, and clarified what else needed to be done on the ones that should be open. Once again, Lee Rowlan (who originally opened #2570593) was helpful and closed the right threads.

Lesson 2: It’d be better if it was easier for the community to resolve open threads in an MR.

I opened or contributed to a number of Slack threads in #contribute about the work, the open questions, the implications of the change, the right semantics of various API changes, how the last bits of test coverage should work, and so on. I spent a while organizing the discussions, trying to track down the necessary answers from the right people. Since Slack is so ephemeral, I would periodically post back to the issue with a transcript of a Slack discussion.

The current hybrid Drupal.org vs. Gitlab integration is both pretty amazing that it works as well as it does, and also leaves a lot to be desired. As someone who maintained the Project* suite of modules that power Drupal.org’s collaboration tools (projects, issues, releases, usage, etc), it’s been interesting to watch the migration to Gitlab. It’s the first big transition for the community’s tools that I’ve been mostly on the sidelines for. I’ve been trying to get a bit more involved again, at least through the Slack channels and meetings. I keep thinking it’d be fun to learn enough Ruby and Go to more meaningfully contribute to getting Gitlab itself to support the collaborative framework we have developed and cultivated in the Drupal community over all these years. There are some real limitations with the current implementation that it’d be great to overcome.

The Tests Really Matter

Part of getting anything into Drupal Core is making sure the test coverage is updated to cover it. If it’s a bug fix, there has to be a failing test that demonstrates the bug before a core committer will merge the changes to solve it. If it’s a new feature, it needs extensive coverage of itself (UI, API, configuration or schema upgrade paths, etc).

In the case of #2570593, one of the early tasks was fleshing out the test coverage for bundle subclasses. The feature could be used for a lot of different things, and the existing tests were only covering some of them. For example, I added tests for using a bundle subclass for User entities (even though User entities don’t have bundles at all).

One of the remaining API details we were sorting out was how the postLoad() method should be invoked. 9.2.x and before would always have Node objects, and if you loaded a bunch of nodes of different types all at once, you still got a giant array of them in your postLoad() implementation and could do whatever you wanted. But now what? If I load many nodes of different types in the same query, would I want my BlogEntry classes to see postLoad() invoked with a bunch of Article, Page, and Blog Entry nodes? Or should it only see the Blog Entry nodes? Article would get a chance to react to all the Article nodes being loaded, and so on.

By comment #157 I thought we had finally resolved everything, I updated the issue summary, and was hoping it might have been RTBC. Thankfully the testbot failed in an unexpected place, proving we had more to do before we had this working properly. An unintended consequence of the implementation of our postLoad() broke the functionality where you can assign different weights to user roles and they would automatically be sorted by weight:

Testing Drupal\Tests\user\Functional\UserRoleAdminTest
.F                                                                  2 / 2 (100%)

Time: 00:13.312, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\user\Functional\UserRoleAdminTest::testRoleWeightOrdering
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    0 => 's9zxarb5'
+    0 => 'anonymous'
     1 => 'authenticated'
-    2 => 'anonymous'
+    2 => 's9zxarb5'
 )

So at comment #163 we find:

Whoops. ;) @larowlan and I stand corrected. The "fix" for postLoad() actively breaks Drupal\user\Entity\Role::postLoad()
public static function postLoad(EntityStorageInterface $storage, array &$entities) {
    parent::postLoad($storage, $entities);
    uasort($entities, 'static::sort');
  }
Once we're invoking postLoad() with $items, not $entities, that sort is only sorting the subarray, but the original $entities array itself is untouched. 😢 That's why https://www.drupal.org/pift-ci-job/2165788 is failing in Drupal\Tests\user\Functional\Views\HandlerFieldRoleTest and related things that are testing for role weights / ordering.

I pushed commit 71a3a3b66f to the MR about it. This undoes the "fix", adds comments about why, and updates the test coverage about method invocations and counts. Here it is in patch form.

Thanks, tests and testbot! Once again, you're saving the day...

That commit added extensive test coverage of method invocations to ensure that core continued to invoke all the right methods on the entity objects when load, update and delete them. This might all seem like a lot of arcane effort, but HandlerFieldRoleTest failing when it should and the resulting improvements prevented untold misery if countless Drupal sites around the world all of a sudden were mysteriously broken and roles no longer “stuck” when you tried to reorder them.

The Framework Maintainers and Release Managers are Amazing

By this point, the issue seemed to be in really good shape. Very nearly RTBC. It was getting a lot of attention and excitement in Slack. Alex Pott took a close look and had extremely helpful and important feedback in #190:

Do we need to do work to make custom / contrib aware of this change. I'm concerned that we're not enough to help classes like SparqlEntityStorage know that they have to change. It's access the entityClass property directly.

There are also storages that are setting it and create objects using it

Should we not consider doing something like https://3v4l.org/UdVmG - we'd need to implement a setter and a magic __set() but at least we could then help classes use getEntityClass() consistently and deprecate direct access to the variable.

As I answered at #195:

Now for the harder stuff... re: @alexpott in #190: This is why you're such a fabulous framework maintainer. 😀 You seem to always think of such things, grep the impact on contrib/custom, etc. Thanks for keeping us honest!

We went on to do exactly what he proposed, added a deprecation layer that automatically noticed if anyone’s code was doing something that would possibly break when we introduced this change. I’m saying “we” because even though I wrote the code and pushed all the commits to Git, numerous folks were helping craft the right approach via Slack and the issue. I opened the follow-up ticket to remove that layer in Drupal 10 once we clean up the remaining technical debt, so we could pepper the code with the right @todo comments that included the link to where they would be resolved.

By #209 we were in the home stretch, and we were trying to perfect the change record, issue summary and other things to make it finally ready to be committed (the original phrase “RTBC” was the acronym for).

To the great delight of myself and countless others, at #211 on October 24th (exactly 2 months after I pushed my first commit to MR 200), @larowlan committed the patch to the 9.3.x branch of Drupal Core. I was very happy that @pfrenssen was credited as that commit’s author, since he wrote the lion’s share of the original functionality. Although by that time I had pushed more commits than anyone else, the heart of the feature was written by Pieter Frenssen. Everyone mentioned in that commit message helped tremendously (and probably other folks, directly or indirectly).

The Regression No One Noticed

Even with all the people that had poured over the code in extreme detail, and the 1000s of automated tests that said it worked, the originally committed change broke some things none of us had noticed. Thankfully @craigra had a test that started failing when it ran against Drupal 9.3.0-alpha1 and @larowlan opened #3246150: Bundle class changes mean the entity class is now determined by the active entity-type definition about it. We sprung into action and converged on a working solution with tests that @catch committed 22 hours after the issue was created. That would be an extreme outlier on a graph of all Drupal Core bug report response times (many of which take years to be fixed). In fairness to the release managers, regressions rarely survive more than a few days before being smashed, so if you were only looking at those, 22 hours is probably only a little better than average. Regardless, the regression was already fixed before 9.3.0-beta1 was released and almost no one was negatively impacted by this oversight. Go Bug Smash Initiative! 🎉

Why Contributing Upstream Matters

Although I “came up with the idea myself”, there’s no way I could have possibly implemented this correctly and completely on my own. Finding the existing issue where it was already basically working shaved a huge amount of time off the effort it took to get this done. I wouldn’t have known about the role weighting surprise without the tests that failed when we tried it. Many of the tricky semantics we had to work out for the Entity API to make this possible required input from folks even more deeply steeped in such matters than I am (@Berdir and others).

As Carl Sagan famously said, “If you wish to make an apple pie from scratch, you must first invent the universe.” This feature is only possible since it builds upon so much prior work, both inside and outside of Drupal.

It seems obvious, but also easy to take for granted: without Drupal Core, there would be no Drupal. Any chance we have to put some time and care into the core system, the impact of that effort is magnified tremendously. All sorts of people with all kinds of backgrounds, talents, skills and resources can contribute, each in their own way. As we say in the labor movement, “An injury to one is an injury to all! A victory for one is a victory for all!”

If you’re struggling with some limitations in Drupal itself (either bugs or gaps in functionality you regularly wished it provided), please try to get involved in fixing that thing “upstream” in the Drupal Core issue queue. For help, you can join Drupal Slack, a local meetup, or whatever other way you have to get involved. The Drupal.org Contributor Guide is extensive.

Of course, if the bug or feature is in a contributed module, that’s a great way to help, too. Contrib projects need all the help they can get (speaking as someone who “maintains” a lot of projects, nearly all of which could benefit from more effort than I can give them myself).

Conclusion

Bundle subclasses can bring object-oriented structure and cleanliness to the majority of the customizations needed for most Drupal sites. They unlock immense power and flexibility without the technical debt of code chaos.

While we were writing this series, TEN7 launched a new version of its own website. During that internal project (which I was almost entirely not involved with), other developers on the team needed a bundle subclass to most cleanly handle some customizations. So this very post is being served by the technology it is describing. Some folks call that “eating your own dogfood”, but I’d rather think of it as “feasting on your own good cooking.” In this case, it wasn’t really “my own” cooking. More accurately, “feasting at an amazing banquet that you played a modest role in helping to prepare.”

I believe that with nearly every aspect of technology, advances can be put to use for good, and for evil. It’s important to consider the ethical and moral implications of new technology before subjecting the world to it. In this case, I’m sure bundle subclasses will be “abused” by some developers and they will make a mess of things. But as technological advances go, the stakes of misuse for this one are very low. If used well, it can make a big improvement. If used poorly, well, “garbage in, garbage out” as they say. The world wouldn’t really be any worse off because of it. I’m proud to have helped bring this change into being.

Overall, I believe this feature can help Drupal developers mature and hopefully become better programmers as they grapple with these concepts. As they start (or continue) to be more actively involved in designing, implementing (and hopefully testing!) their own APIs, they will learn what works and what doesn’t. There’s nothing like first hand experience to help you understand something.

We’ve been thrilled with the results of working with bundle subclasses. We hope you give them a try! May this series also inspire you to contribute to improving the tools you use, in whatever ways you can.