Why it is a bad idea to do function calls on injected services during instantiation in Drupal 8

Today I was tracking down a strange issue with a form submission and validation, which finally turned out to be the consequence of an unobvious wrong function call inside a class constructor.

While I was trying to solve a minor cosmetical bug within Drupal Commerce checkout flow configuration UI, I was experiencing strange problems on form validation and submission like:

Fatal error:  Call to a member function submitConfigurationForm() on integer in /var/www/drupalvm/web/modules/commerce/modules/checkout/src/Plugin/Commerce/CheckoutFlow/CheckoutFlowWithPanesBase.php on line 457

I knew, that yesterday it worked without problems, And Bojan from Commerce Guys witnessed, that he can't reproduce this bug at all on a clean install. The only clear difference were my custom checkout panes, that I have developed so far. So I removed them all at first, asserting that the form was still working. Then I started to re-add them one by one, until I've found the errorneous one. I was debugging through different stages of form building, validation and submitting, seeing strange things, like broken form validation callback declarations and wrong properties of the form state, carrying arrays of my checkout pane's properties instead of the instantiate pane object itself.

After I removed several functions of my pane and completely eroded the internals of the only abstract function a pane has to implement, the error still occurred. I checked the class annotations for the xth time - everything ok. The only remaining stuff was the constructor and static factory method, that injects the needed services into my plugin. So I removed them all at first, and BOOM! No error anymore.

So I started to re-add the services one by one: the entity type manager, the renderer and the logger factory. And now I saw, what I've done wrong. As I didn't want to write $this->loggerChannelFactory->get('channel_name')->warning('message'), but instead preferred to use $this->logger->warning('message'), I did the following:

As I found it an overkill to declare another custom service to directly inject a specific channel as described in the official documentation, I decided to inject the logger factory, but call the getter function on it to retreive a specific channel during instantiation - calling $container->get('logger.factory')->get('channel_name') inside of the factory function.

The really mean thing here is, that it basically works. There's absolutely no problem during checkout at all. But it turned out, that it is getting a problem inside the configuration form, where the pane objects are also instantiated. And the part, where the settings are changed, is called via Ajax. I guess, that the Ajax call was the reason, why the problem arised here and not during checkout.

Finally I've changed to set the channel factory as a class property of my plugin and do $this->loggerChannelFactory->get('channel_name') instead and the errors were gone.

Lessons learned

  1. Never do function calls on injected services during instantiation of a plugin, when the plugin is loaded during an Ajax form callback.
  2. Better: never do function calls on injected services during instantiation of a plugin at all. Try to find a better way. Only do it, when you really know, that the underlying is unproblematic - the given example call itself is calling other services, which is/can be problematic.

Neuen Kommentar schreiben