Problem/Motivation

After working on #3494818: Improve error handling related to Drupal.org endpoint, where we were forcing errors, we realised that the errors are cached, even after these are fixed. We thought about invalidating the keyvalue in that case, but we'd need to invalidate it for all plugins, instead of just the failing one, which would be really unefficient.

Steps to reproduce

Force an error in a plugin, visit the browsing page, then fix it and visit the browsing page again, and the error message still displays.

Proposed resolution

Cache results only per plugin instead of in a global object.

Currently, in src/EnabledSourceHandler.php:
$this->keyValue->set($cache_key, $stored);

Issue fork project_browser-3498231

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

fjgarlin created an issue. See original summary.

fjgarlin’s picture

fjgarlin commented
Issue summary: View changes

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

narendrar opened merge request !666

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Active » Needs review
fjgarlin’s picture

fjgarlin commented

I’m having a look at the code and leaving some comments in the MR. Either I’m not understanding the logic too well or I think we are losing too much “cached” information with this approach. I’ll leave a comment on the issue to have better track of things.

Right now (before this issue), we are storing all results for all plugins for a particular query in a keyvalue object, all under the same key, for all plugins.

With this MR, we are storing under that same keyvalue object, only the results of one plugin, so if we are going back and forth between plugins, we'd need to recalculate things.

I think it'd make more sense to have the "key" of the object being specific to the query and the source. If the call is successful, then store it, and if it isn't (ie: $results->error is not empty), then don't store it (so it goes through the full external query again when the same data is required).

Possible approach:
getProjects says Returns projects that match a particular query, from all enabled sources.
We start and check the cache there, and I think it's wrong, because maybe one plugin was not working at the time.
We'd need to loop through the sources (quick) and then get the results, which may or may not be cached. ==> this is where we need to introduce the caching, per plugin, not globally.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented

Updated method comments and changes done as suggested.
I think this is ready for review again.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Thanks @narendrar!

This is definitely a step forward to untangling the "everything is a single agglomerated set of tabs" architecture that is the albatross around Project Browser's neck. There is more I'd like to do, but it can happen in a follow-up. Baby steps is what it's all about.

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Fixed

Whew!!!

Status: Fixed » Closed (fixed)

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