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

CommentFileSizeAuthor
#16 3017255-15.patch1.48 KBe0ipso
#2 3017255-2.patch1.06 KBndobromirov
#6 issue-3017255-interdiff-6-2.txt859 bytesndobromirov
#6 issue-3017255-6.patch1.07 KBndobromirov
#11 static_cache-3017255-11.patch915 bytesrhristov
#7 issue-3017255-7.patch859 bytesndobromirov

Comments

ndobromirov created an issue. See original summary.

ndobromirov’s picture

ndobromirov commented
Status: Needs work » Needs review
StatusFileSize
new 3017255-2.patch1.06 KB

Here is the proposed solution.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Great! Thanks for the patch!

You should look into JSON:API Extras 3.x

  • e0ipso committed 94356e0 on 8.x-2.x authored by ndobromirov 
    Issue #3017255 by ndobromirov, e0ipso: Add a static cache on the...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed
ndobromirov’s picture

ndobromirov commented
Status: Fixed » Needs review
StatusFileSize
new issue-3017255-interdiff-6-2.txt859 bytes
new issue-3017255-6.patch1.07 KB

Sorry my bad doing patch refactoring in the end... Here is a fix that will actually work.

ndobromirov’s picture

ndobromirov commented
StatusFileSize
new issue-3017255-7.patch859 bytes

Ops did not rebase the module. Here is a rebased version of the patch (same as the interdiff above).

The last submitted patch, 6: issue-3017255-6.patch, failed testing. View results

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed

Ugh! Sorry for the sloppy review earlier. Thanks for looping back.

  • e0ipso committed 11c96cc on 8.x-2.x authored by ndobromirov 
    Issue #3017255 by ndobromirov, e0ipso: Add a static cache on the...
rhristov’s picture

rhristov commented
Status: Fixed » Needs review
StatusFileSize
new static_cache-3017255-11.patch915 bytes

The 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.

rhristov’s picture

rhristov commented
ndobromirov’s picture

ndobromirov commented
Status: Needs review » Reviewed & tested by the community
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Because how the drupal_check_incompatibility is working this check could produce unexpected results

I'd love to understand more about this. Why is it unreliable?

  • e0ipso committed 39f1e14 on 8.x-2.x 
    Issue #3017255 by ndobromirov, rhristov, e0ipso: Add a static cache on...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new 3017255-15.patch1.48 KB

I believe this was failing due to -dev versions not getting a real version number internally :-(

Hopefully this patch fixes that.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.