- 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.
- ๐ฎ๐ชIreland lostcarpark
Looked at this this morning, and asked, "who put Kev-Value in the title?", and it appears that I did.
- Merge request !625Add Drush command and UI button to clear non-volatile storage โ (Open) created by chrisfromredfin
- ๐ฉ๐ช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 aFormBase
, 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 extendsFormBase
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) - ๐บ๐ธ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.
- ๐บ๐ธ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.
- ๐บ๐ธUnited States pfrilling Minster, OH
All new code should be under test coverage now.