The right tools?

Usually when I upgrade Magento, the procedure outlined above works fine. My tests are spread across unit tests, integration tests and functional tests (ran using PHPUnit with a copy of the production database). Also, there are PHP linters, PHPCS, PHPStan and PHPMD. And the obvious bin/magento setup:di:compile. I trust that these tools spot a lot of bugs, before ending up on production. Still, after going live, there are the obvious manual tests as well, just to make sure everything is running fine.

Actually, after going live, I'm even running some simple Cypress tests to check for the right output. And this final step, after the Magento 2.4.7 release was put in production, failed. My GraphQL API was down - and with it, the Yireo Courseware Portal and a less vital microservice used by my custom CRM.

First of all, let me state that I'm going to deal with things soon a bit different: My Cypress tests are going to be run on the staging environment, duh.

Let's move on to the actual bug. That's where the stuff really gets funny.

The GraphQL error

The annoying thing about the GraphQL error was that the GraphQL API (as used via a client) simply reported an internal error, without reporting any further details. And this happened for any query being made.

This is where I then open up the logs, but those logs were empty: Even with PHP error reporting configured properly, in the Developer Mode, etc. It's still something I would like to research, because I personally think that in the Developer Mode analysing a bug like this should be easier.

Quick fix

Anyway, I was stressed out. But after testing things quickly in a plain Magento site, I concluded that the issue might very well be related to a non-core module. A quick scan revealed that the only non-core modules that I was using were created by the mysterious vendor Yireo.

Quickly after this, I concluded that the culprit was my Yireo_CustomGraphQlQueryLimiter module: A module that modifies the default query processing of Magento to allow for the query limits to be configured in an easier way. I'm not going to explain more, the README of the module explains more.

The point is not about the quick fix or the module. The point of this blog is about the cause of this error.

What changed?

In Magento 2.4.6 or older, the class \Magento\Framework\GraphQl\Query\QueryProcessor had a process method with the following signature:

public function process(
    Schema $schema,
    string $source,
    ContextInterface $contextValue = null,
    array $variableValues = null,
    string $operationName = null
): array {}

In Magento 2.4.7, this changed towards the following:

public function process(
    Schema $schema,
    DocumentNode|string $source,
    ContextInterface $contextValue = null,
    array $variableValues = null,
    string $operationName = null
): array {}

In other words, the $source argument - which always used by of type string - could now also be of type DocumentNode. This changed in commit b0db11f605951a25f4d17b0c33069adbcc840cb1 which is part of a PR 12da28cfc271f2e3e535aa66a4bedbc746d662a1 of which Damien Retzinger mentioned: "This change likely saves at least 30ms (for me) per request!".

It sounds like good stuff. But it is also breaking my DI plugin.

My DI plugin

In the Yireo_CustomGraphQlQueryLimiter module, the process() method is intercepted as follows:

public function beforeProcess(
    QueryProcessor $queryProcessor,
    Schema $schema,
    string $source,
    ContextInterface $contextValue = null,
    array $variableValues = null,
    string $operationName = null
) {}

Note that the type hint in this case is still string, so not compatible with the new union type DocumentNode|string of Magento 2.4.7.

Weird note: DI compile worked

It is interesting to note that bin/magento setup:di:compile worked fine - no compilation errors! The reason for this is that the ObjectManager magic that compiles the interceptor does not take type hints into account, so therefore ignores the incompatible types.

Only in runtime things break.

Why was this not detected with semantic versioning?

In the Yireo module, dependencies are - to my personal understanding - properly declared. For instance, there is a dependency in the composer.json file with the magento/framework package with the following semver constraint: ^100.1|^101.0|^102.0|^103.0.

Unfortunately, in Magento 2.4.6, the framework has been released with version 103.0.6 (with a few patches) and in Magento 2.4.7, the framework has been released with version 103.0.7. Only the patch number incremented.

This, while according to the compatibility standards - as I understand it - any change in signatures should be seen as a breaking change. Or is it ...? The \Magento\Framework\GraphQl\Query\QueryProcessor class is not marked with @api so does not classify as a service contract. My bad?

Lesson learned: DI plugins are error-prone

First of all, I learned from this that DI plugins are error-prone. Especially if you depend upon classes that are not marked as service contract. Furthermore, because the DI compiler ignores some parts of typehinting (I'm not totally knowledgable here), typehinting issues like these are not detected by DI compilation.

Lesson learned: Magento still does not apply semantic versioning?

Even though I thought the typehinting change was a breaking change, Magento did not see it that way. The QueryProcessor class was not marked as API, so therefore should not have been modified anyway? I'm not sure how to react to this: I personally would say that any code in the framework is ready for modification elsewhere, but hey, it's their code.

Mainly, in these cases, I can't rely upon semantic versioning.

Lesson learned: You need tests

However, issues like these are detected with tests: Either integration tests, functional tests or end-to-end tests. I actually had these tests lying around, but I either ran them in the wrong place (the Yireo module tests were not run for going live with my updated shop) or too late (Cypress tests after going live, instead of on staging).

And if you expect that Magento is not always playing ball with semantic versioning, plus the fact that DI compilation did not fail, the only way to detect this bug beforehand is to run the right test.