Problem/Motivation

KlaroHelper::matchKlaroApp() runs 22 times on the Drupal CMS (starter recipe) front page.

Every time it runs, it executes an entity query for klaro app config entities.

The results of the entity query could be statically cached in a class property (or using the memory cache backend) so that it only runs per request.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork klaro-3578931

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

jan kellermann’s picture

jan kellermann commented
Version: 3.0.8 » 3.1.x-dev
Assigned: Unassigned » jan kellermann

We could add caching here:
https://git.drupalcode.org/project/klaro/-/blob/3.x/src/Utility/KlaroHel...

The cache key must take the given parameters into account.

kazajhodo made their first commit to this issue’s fork.

kazajhodo’s picture

kazajhodo commented

Added caching for getApps() result, keyed by arguments, per request, with test.

kazajhodo’s picture

kazajhodo commented
Status: Active » Needs review

jan kellermann opened merge request !137

jan kellermann’s picture

jan kellermann commented
Assigned: jan kellermann » Unassigned

Looks good, Thank you very much.

I fixed a small PHPCS warning and opened the MR.

Waiting for RTBC. @catch ?

kazajhodo’s picture

kazajhodo commented

Np, nice!

catch’s picture

catch
he/him
Primary language English
commented

It would be slightly better to inject the cache.memory bin and use that rather than a raw protected property - e.g. theoretically would allow the entity list cache tag to be added to the cache item so that changes in the same request are reflected automatically, although not sure that would be a real issue in practice.

kazajhodo’s picture

kazajhodo commented

I think usually this is the right call; but I also agree with your leaning... this likely is not a real issue in this specific case.

The tagged bin would self-heal, but on an admin form save its not possible to run into the issue, right? We'd be adding dependencies for a case we'd never encounter.

However, you are a wizard, so if you think its worth, I'll add it in.

daften’s picture

daften commented
Status: Needs review » Reviewed & tested by the community

I tested this on a project where Klaro was having quite a bit of impact due to the amount of fields, and this worked great. No issues at all. In fact I had test-driven the same basic change locally before finding this MR.

Thanks!

jan kellermann’s picture

jan kellermann commented

@catch - what is your opinion? RTBC or back to needs work regarding to your comment 9?

catch’s picture

catch
he/him
Primary language English
commented

@jan I think that's optional and could be done in a follow-up.

jan kellermann’s picture

jan kellermann commented
Status: Reviewed & tested by the community » Fixed

Thank you all for your work!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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