Michael G

A story of contributing a fix to Drupal… and a pragmatic workaround

When I upgraded a site from Drupal 10.1 to 10.2, I discovered a particularly serious bug: the login form on our client’s site vanished … which was pretty serious for this site which hid all content behind a login!

We had a custom text format filter plugin to render the login form in place of a custom token in text that editors set, on one of the few pages that anonymous users could access. Forms can have quite different cacheability to the rest of a page, and building them can be a relatively expensive operation anyway, so we used placeholders which Drupal can replace ‘lazily’ outside of regular caching:

class MymoduleLoginFormFilter extends FilterBase implements TrustedCallbackInterface {

  public function process($text, $langcode) {
    $result = new FilterProcessResult($text);
    $needle = '[login_form]';
    // No arguments needed as [login_form] is always to be replaced with the same form.
    $arguments = [];
    $replace = $result->createPlaceholder(self::class . '::renderLoginForm', $arguments);
    return $result->setProcessedText(str_replace($needle, $replace, $text));
  }

  public static function renderLoginForm() {
    // Could be any relatively expensive operation.
    return Drupal::formBuilder()->getForm(UserLoginForm::class);
  }
  
  public static function trustedCallbacks() {
    return ['renderLoginForm'];
  }

}

But our text format also had core’s “Correct faulty and chopped off HTML” filter enabled – which completely removed the placeholder, and therefore the form went missing from the final output!

Debugging this to investigate was interesting – it took me down the rabbit hole of learning more about PHP 8 Fibers, as Drupal 10.2 uses them to replace placeholders. Initially, I thought the problem could be there, but it turned out that the placeholder itself was the problem. Drupal happily generated the form to go in the right place, but couldn’t find the placeholder. Here’s what a placeholder, created by FilterProcessResult::createPlaceholder() should look like:

<drupal-filter-placeholder callback="DrupalmymodulePluginFilterMymoduleLoginFormFilter::renderLoginForm" arguments="" token="hqdY2kfgWm35IxkrraS4AZx6zYgR7YRVmOwvWli80V4"></drupal-filter-placeholder>

Looking very carefully, I spotted that the arguments="" attribute in the actual markup was just arguments – i.e. it had been turned into a ‘boolean’ HTML attribute:

<drupal-filter-placeholder callback="DrupalmymodulePluginFilterMymoduleLoginFormFilter::renderLoginForm" arguments token="hqdY2kfgWm35IxkrraS4AZx6zYgR7YRVmOwvWli80V4"></drupal-filter-placeholder>

There is a limited set of these, and yet the masterminds/html5 component that Drupal 10.2 now uses to process HTML 5 requires an explicit list of the attributes that should not get converted to boolean attributes when they are set to an empty string.

At this point, I should point out that this means a simple solution could be to just pass some arguments so that the attribute isn’t empty! That is a nice immediate workaround that avoids the need for any patch, so is an obvious maintainable solution:

// Insert your favourite argument; any value will do.
$arguments = [42];

At least that ensures our login form shows again!

But I don’t see any documentation saying there must be arguments, and it would be easy for someone to write this kind of code again elsewhere, especially if we’re trying to do The Right Thing by using placeholders in filters.

So I decided to contribute a fix back to Drupal core. I’ve worked on core before. Sometimes it’s a joy to find or fix something that affects thousands of people, other times the contribution process can be soul-destroying. At least in this case, I found an existing test in core that could be easily extended to demonstrate the bug. Then I wrote a surgical fix… but I can see that it tightly couples the filter system to Drupal’s HtmlSerializerRules class. That class is within the DrupalComponent namespace, which is described as:

Drupal Components are independent libraries that do not depend on the rest of Drupal in order to function.

Components MAY depend on other Drupal Components or external libraries/packages, but MUST NOT depend on any other Drupal code.

So perhaps it needs configuration in order to be decoupled; and/or a factory service; or maybe modules should subscribe to an event to be able to inject their own rules …. and very quickly perfection feels like the enemy of good, as I can imagine the scope of a solution ballooning in size and complexity. 

I’m all for high standards in core, but fulfilling them to produce solutions can still be a slow and frustrating experience. I’m already involved in enough long-running issues that just bounce around between reviewers, deprecations and changes in standards. I risk just ranting here rather than providing answers – and believe me, I’m incredibly grateful for the work that reviewers and committers have put into producing Drupal – but surely the current process must be putting so many potential contributors off. We worry about attracting talent to the Drupal ecosystem, and turning Takers into Makers, but what are they going to find when they arrive? Contributing improvements of decent size is hard and can require perseverance over years. Where can we adjust the balance to make contribution easier for anyone, even seasoned developers?

As I suggested, perhaps this particular bug needs any of a factory pattern, event subscriber, or injected configuration… but what would my next step be? I’m reluctant to put effort into writing a more complex solution when I know from experience that reviewers might just suggest doing something different anyway. At least I have that simple (if unsatisfying) workaround for the filter placeholder method: always send an argument, even if it might be ignored. I guess that reflects the contribution experience itself sometimes!