Allow clearing of cached data from Project Browser sources

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.

chrisfromredfin → credited fjgarlin → .

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.

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

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?

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.

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

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)

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.

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.

All new code should be under test coverage now.

Rebased this and tested and it works great. Tests pass. However, I'm unsure about the use / need for php-mock. Doesn't core have a supported way to do mocks without this library? OR, does core use this library and we just don't yet?

The php-mock library was just used to mock the global `function dt()` from Drush. The Core documentation I found for mocking these types of functions is here: https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/unit-tes... → , but I honestly always feel like I'm doing something wrong when mocking the global functions that way.

My personal preferences aside, I can rewrite the tests to remove that extra package if you feel strongly about it.

I think that Drush has its own way to mock those things, actually - which would also mean we can remove the package.

@see
Drush Test Traits - https://www.drush.org/13.x/contribute/unish/#drush-test-traits
-and-
https://git.drupalcode.org/project/devel/blob/8.x-2.x/tests/src/Function...

...maybe?

I think I hit a bit similar issue today when I was testing the Drupal CMS RC2.

The first one was related to a contrib module logo.png. Yesterday I switched the module default branch from 1.1.x to 1.2.x. The 1.2.x branch had the new logo.png. Drupal.org module page was able to pick up the new logo.png after the cronjob in Drupal.org was ran. But I wasn't able to get the logo.png refreshed in Drupal CMS. At the end I was able to solve this by uninstalling and installing the Project Browser again which most likely also cleared the key/value storage.

The second issue was a bit more interesting. Basically what happened is we had a 1.1.1 release of the contrib module and that installed fine through Project browser. Today I released a new stable 1.2.0 version of the module. This stable version installed fine through Composer but when I tried to install the module through Project browser, it gave me an error that there is no stable release 1.2.0 available. This made me wonder if Project browser also saves the available releases information in the storage? Unfortunately I wasn't able to capture that error and it's now a bit hard to reproduce it without making a new release for the module. But again this new version installed fine after the uninstall & install steps for PB.

The logo problem is a small nuance but I wonder if the release problem might affect less technical users wanting to install and try out modules? It might be a tall order to make some less technical person understand that they need to clear some cache to get rid of the error. That being said, it might be that I just hit some other Project browser related issue when installng the new version but thought this might still be valuable information.

I updated the drush test to be like the examples you provided @chrisfromredfin. The new test is passing, however, I seem to have broken phpcs/phpstan checks after my rebase. Any thoughts, or do I need to revert and try again?

I think that phpcs isn't liking that [CLI\SomeAttribute] syntax. You'll want to fully import each attribute, not just the CLI namespace, and then convert those attributes to just #[SomeAttribute]. See if that shuts it up.

Thanks @phenaproxima! That did the trick!

I went ahead and fixed the phpstan warnings too. I believe this is now ready for review (I believe the Nightwatch failures are expected).

I think the Nightwatch tests have been removed, so you can just sync your branch with 2.0.x to get rid of those, I believe.

All tests are now green!!

This is really clear and well-tested. But I think it could be simpler. We don't need a whole separate form, route, and tab for this; let's just add a button to the existing settings form. And we don't need three tests; one test, with three methods, should suffice.

Throwing everything into a single form raises again the problem outlined in #9 ✨ Allow clearing of cached data from Project Browser sources Active :/

When a problem comes along, you must ship it!

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