Allow clearing of cached data from Project Browser sources

Created on 23 August 2024, 4 months ago

Problem/Motivation

Project Browser caches data from sources as key/value pairs. When we were using the mocked API data this wasn't much of an issue, but with the API data there can be a need to clear the cached data.

Steps to reproduce

Proposed resolution

During discussion on Slack, @fjgarlin suggested the following options:

  • Moving keyvalue storage to cache storage
  • Using keyvalue.expirable, which has a setWithExpire method
  • Offer an option in the PB admin settings form to clear all the stored keyvalue values. That would just call the ->deleteAll(); for the project_browser set of values.

Personally, I think we should try to ensure that the cached data can be cleared with the standard Clear Cache in performance settings or drush cr, so I feel we should either move into cache storage, or hook into the Drupal cache clear command, which I think we can do by implementing \Drupal\Core\Plugin\CachedDiscoveryClearerInterface.

โœจ Feature request
Status

Active

Version

2.0

Component

Code

Created by

๐Ÿ‡ฎ๐Ÿ‡ชIreland lostcarpark

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lostcarpark
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    There is a reason we're using key-value storage - we don't want this stuff to be volatile. The problem with data stored in a regular cache is that it can disappear at any time and become inconsistent.

    That said, I am on board with having some mechanism for clearing the data store. I like these:

    • Using keyvalue.expirable, which has a setWithExpire method
    • Offer an option in the PB admin settings form to clear all the stored keyvalue values. That would just call the ->deleteAll(); for the project_browser set of values.
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States chrisfromredfin Portland, Maine

    Given this input, I think we should provide a mechanism through admin/config/devel/project-browser settings page to "cleared stored project data" or something. A button that acts like the cache clear button does.

    I think it's also worthwhile to add a separate Drush command that will clear it. This is mostly for DX, so I think tucking it behind its own Drush command is adequate. Updating IS with a plan.

    Marking as a stable blocker, but as it's DX, it would be nice to have sooner rather than later.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States chrisfromredfin Portland, Maine
  • ๐Ÿ‡ฎ๐Ÿ‡ชIreland lostcarpark

    Looked at this this morning, and asked, "who put Kev-Value in the title?", and it appears that I did.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States chrisfromredfin Portland, Maine
  • Pipeline finished with Failed
    about 1 month ago
    Total: 554s
    #345265
  • Pipeline finished with Success
    about 1 month ago
    Total: 748s
    #345276
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany rkoller Nรผrnberg, Germany

    one immediate reaction/thought, when i went to admin/config/development/project_browser, it feels sort of wrong to have a form element, the clear storage fieldset, right after a save configuration button (a form sort of ends with the save configuration button - the appearance page also sort of breaks with that paradigm in a different but similar way). one thing we tried to mind during ux meetings was a page shouldnt cover too many different topics and tasks. at the moment you are able to enable the experimental setting, then you have the setting for enabling and disabling the source types which the save configuration button refers to, and then the newly introduced clear storage button wrapped in a fieldset. i would rather tend to suggest to have at least two local tasks, one for the checkbox and the table and the other for the clearing the storage fieldset?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States chrisfromredfin Portland, Maine

    I'll probably change it when core changes that on the development page and moves the clear cache button to its own tab? In the meantime, I'll continue to follow the existing patterns of core.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States chrisfromredfin Portland, Maine

    (Eating words at least a little) - per conversation with @phenaproxima in Slack, even though core sets a precedent for a two-form pattern (https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...), it doesn't mean we should use it.

    Instead, we should be able to add a second #type `button` or #type `submit` to the form and then figure out how to branch which logic runs based on which button was pressed.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain fjgarlin

    Second button and trigger another method is easy, however, if the button is inside a ConfigFormBase instead of a FormBase, some modules like config_readonly โ†’ (incredibly useful in production environments) might have a problem, since any action inside a config form will be disabled.

    If the action is not going to change configuration it shouldn't be inside a configuration form in my opinion. It's currently done this way in the MR.

    We could:
    - Create a new tab/page with the form proposed in this MR (which extends FormBase correctly). If desired, we can add a link from the settings page to this new tab/page.
    - Maybe add the button to the /admin/config/development/performance page (which again, can be linked from the PB settings page if needed)

  • Pipeline finished with Success
    about 1 month ago
    Total: 451s
    #346207
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 193s
    #346222
  • Pipeline finished with Failed
    about 1 month ago
    Total: 401s
    #346223
  • Pipeline finished with Failed
    about 1 month ago
    Total: 456s
    #346224
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 191s
    #346233
  • Pipeline finished with Success
    about 1 month ago
    Total: 356s
    #346236
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States chrisfromredfin Portland, Maine

    If anyone has any suggestions on other tests to write, please let me know. I will need help with the architecture/test design but I'm fairly comfortable with the syntax.

  • First commit to issue fork.
  • Pipeline finished with Canceled
    10 days ago
    Total: 182s
    #366045
  • Pipeline finished with Failed
    10 days ago
    Total: 364s
    #366048
  • Pipeline finished with Failed
    10 days ago
    Total: 447s
    #366049
  • Pipeline finished with Failed
    10 days ago
    Total: 474s
    #366639
  • Pipeline finished with Canceled
    10 days ago
    Total: 128s
    #366671
  • Pipeline finished with Success
    10 days ago
    Total: 323s
    #366672
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States pfrilling Minster, OH

    I added unit tests to the ActionsForm class as well as the Drush command.

    I think we still need test coverage on the clearStorage() method.

  • Pipeline finished with Success
    6 days ago
    Total: 437s
    #369996
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States pfrilling Minster, OH

    All new code should be under test coverage now.

Production build 0.71.5 2024