Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
It's doing version parsing and comparison. Even if it's a simple one, once you start calling it ~1k times it will take time.
In my case it's responsible for 0.7% of the total request time or 30ms.
Proposed resolution
My assumption is that varsion will not change mid-request, so adding this as a static cache variable should be OK.
Remaining tasks
Patch, benchmarks commit.
public static function isJsonApi2x() {
static $result = NULL;
if ($result === NULL) {
$v = ModuleHandler::parseDependency('jsonapi(>= 8.x-2.0-beta1)');
$module_list = \Drupal::service('extension.list.module')->getList();
$result = is_null(drupal_check_incompatibility($v, $module_list['jsonapi']->info['version']));
}
return $result;
}
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3017255-15.patch | 1.48 KB | e0ipso |
| #11 | static_cache-3017255-11.patch | 915 bytes | rhristov |
| |||
| #7 | issue-3017255-7.patch | 859 bytes | ndobromirov |
| |||
Comments
Comment #2
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere is the proposed solution.
Comment #3
e0ipsoGreat! Thanks for the patch!
You should look into JSON:API Extras 3.x
Comment #5
e0ipsoComment #6
ndobromirov CreditAttribution: ndobromirov at FFW commentedSorry my bad doing patch refactoring in the end... Here is a fix that will actually work.
Comment #7
ndobromirov CreditAttribution: ndobromirov at FFW commentedOps did not rebase the module. Here is a rebased version of the patch (same as the interdiff above).
Comment #9
e0ipsoUgh! Sorry for the sloppy review earlier. Thanks for looping back.
Comment #11
rhristov CreditAttribution: rhristov at Bulcode commentedThe applied static cache and the method, in general, is not working, it is returning always true now and it breaks the automated tests because they are running against JSONAPI v1, you can see what is happening for example here: https://www.drupal.org/pift-ci-job/1139001.
Because how the drupal_check_incompatibility is working this check could produce unexpected results, so I have added simple and fast check relying on if one service introduced in v2 is available.
This issue is currently blocking the tests from https://www.drupal.org/project/jsonapi_extras/issues/2997956#comment-128... to succeed and merging jsonapi_defaults into extras is really important for one of our projects so I will appreciate moving this task forward.
Thanks.
Comment #12
rhristov CreditAttribution: rhristov at Bulcode commentedComment #13
ndobromirov CreditAttribution: ndobromirov at FFW commentedComment #14
e0ipsoI'd love to understand more about this. Why is it unreliable?
Comment #16
e0ipsoI believe this was failing due to -dev versions not getting a real version number internally :-(
Hopefully this patch fixes that.