Make all enabled sources exposed as local tasks

I'll deal with this.

This is ready for review.

The current test failures (Nightwatch) are preexisting; it's just that they were hidden behind "allowed to fail" jobs that test against the next minor of core; see https://git.drupalcode.org/project/project_browser/-/jobs/3668368#L194. It's just that, now that a new minor of core has been released and GitLab CI has adjusted, those "allowed to fail" jobs are now actually failing.

So I don't think that should block commit.

lol not me testing this MR, staring at Browse trying to figure out where Recipes are, then seeing it right next to it at the top level. that's the point!

UI looks good, and I reviewed the code and the tests. Great work!

I think doing this breaks a somewhat-fundamental (albeit poor) assumption that this would always be a bit of a SPA. So, I think we need to introduce state handling or something into the front-end to make sure that we're clearing previously-stored tab state if switching routes, otherwise you get to the "Recipes" route and still see previous iterations of the "Modules" tab.

VIDEO DEMO: https://www.drupal.org/files/issues/2024-12-18/Monosnap%20screencast%202... →

Also, put Recipes _after_ Browse in the tab order.

I'll open a follow-up to see if we want to change "Browse" to "Modules" or change both to "Browse Modules" and "Browse Recipes" - but those shouldn't hold anything.

That's a sad, but it is what @pameeela saw too when testing the current tabs (which we have hacked together in Drupal CMS).

I suspect the sadness is mostly in the onMount() hook of the ProjectBrowser Svelte component - it does a bunch of weirdo stuff with the active tab.

I discussed this with @tim.plunkett and @chrisfromredfin on Slack and we officially agreed that the Svelte tabbing is much more trouble than it's worth.

Let's rip it out. Right here, right now. Let the bloodbath begin! This might cause merge conflicts with virtually every other active MR that touches the Svelte code, but I'm hoping that once the tabbing has been cast into the abyss, the Svelte code will generally be simpler, cleaner, more comprehensible, and more maintainable.

The long thread where this was decided: https://drupal.slack.com/archives/C07G41EUJP4/p1734529783367599

Giving this a review & test per a discussion today with @phenaproxima

Here, per @chrisfromredfin's request, is a brain-dump of what I'm thinking after emerging from the other side of this MR.

Although I haven't fully succeeded in casting the Svelte tabbing into the abyss, I have definitely succeeded in beheading the basilisk.

I think I've realized something: most of the complexity in Project Browser can be traced to the Svelte tabbing. Having it work that way necessitates all kinds of strange, complicated state management on the client side. Combined with Project Browser's roots as a proof of concept, it makes for a stunning degree of awkwardness.

This is the first step in removing that awkwardness, taming the complexity, and making the code and user experience cleaner, faster, and more testable. There is a long way to go.

The ultimate vision, I think, is to have the ability to put several instances of Project Browser on a single page, arranged as blocks in a layout, and each showing projects from a particular source with a particular set of filters (or no filters at all) and sorts. In a limited way, it's similar to Views, but where Views deals with Drupal's data model and stretches the field and rendering systems to their limit, we are dealing with a JSON API endpoint and rendering using bespoke JavaScript components (which I think makes sense and is a sound approach).

The current Svelte code would not work if you tried to have several instances of Project Browser in a block layout. It has fundamental assumptions which are incompatible with that. The presence of tabbing was one of them; that assumption is now gone, although the word "tab" is still littered throughout the code. (Renaming everything and cleaning it up would have made this MR impossibly huge, and utterly unreviewable.)

So where do we go from here?

To begin with, I think we need a new client-side data model. The ProjectBrowser.svelte component, in particular, doesn't really behave like a component at all - it behaves like a main entry point. That's inappropriate. We should be able to do this instead (this is pseudocode, but I think it illustrates how I think this should work):

// This is what the PHP code to define a project browser instance would look like in the backend:
$browser = [
    '#type' => 'project_browser',
    '#source' => 'drupalorg_jsonapi',
    '#instance_id' => '7f0ba984-0831-434f-b360-78231c494485',
];

// That render element would turn into this:
drupalSettings.projectBrowserInstanceInfo = {
    "7f0ba984-0831-434f-b360-78231c494485": {
        "source": "drupalorg_jsonapi",
        "filterDefinitions": {
            "securityCoverage": {
                "_type": "boolean",
                // ...etc.
            },
            "maintenanceStatus": {
                "_type": "boolean",
                // ...etc.
            },
            // There could be more filter definitions here.
        },
        "sortOptions": {
            "relevant": "Most relevant",
            "a-z": "A-Z",
            "z-a": "Z-A",
            // ...etc.
        },
    },
    // There could be another instance here
};

// Any filters and sorts previously chosen by the user *FOR THIS INSTANCE* are restored from session storage.
const instance = mergeWithSessionStorage(
    drupalSettings.projectBrowserInstanceInfo["7f0ba984-0831-434f-b360-78231c494485"]
);

// After that, here's what `instance` would look like:
{
        "source": "drupalorg_jsonapi",
        "filters": {
            "securityCoverage": {
                "_type": "boolean",
                "value": true,  // This is either the default value, or whatever the user has previously selected (from sessionStorage).
                // ...etc.
            },
            "maintenanceStatus": {
                "_type": "boolean",
                "value": false,
                // ...etc.
            },
            // There could be more filter definitions here.
        },
        "sortOptions": {
            "relevant": "Most relevant",
            "a-z": "A-Z",
            "z-a": "Z-A",
            // ...etc.
        },
        "sortBy": "z-a",  // Either a default value, or whatever the user has previously selected (from sessionStorage).
}

// So then we just pass all of that to a ProjectBrowser component.

<div data-project-browser-instance-id="7f0ba984-0831-434f-b360-78231c494485">
<!--If the filters or sortOptions are null, then the instance won't display any filters or sort options.-->
<ProjectBrowser
    source=${instance.source}
    title="Recommended Modules"
    filters=${instance.filters}
    sortOptions=${instance.sortOptions}
    sortBy=${instance.sortBy}
/>
</div>

In other words, let's ditch this entire concept of an "active tab". A project browser should simply be a component, with parameters that are passed into it. It's the main application's job to assemble those parameters, by combining whatever is in sessionStorage with the backend information about a given instance.

We should also get rid of constants.js. We shouldn't be assembling everything from constants that are really just cherry-picking parts of drupalSettings (or sessionStorage, depending on various hard-to-grok state flags). There should only be a single point in the application that is responsible for assembling the complete information about an instance of Project Browser, and passing that on to the component for rendering.

I also think that there should be two centralized "services" available to all components in the Project Browser app -- think of them as client-side versions of \Drupal::service(). We need a query service, and an installer service.

So, if a ProjectBrowser components wants to retrieve information about projects from the source, it would do something like this (again, pseudocode):

import QueryManager from './QueryManager.js';

export let source;
export let filters;

const projectsData = await QueryManager.query(source, filters);

// projectsData looks pretty much like what we return now -- a list of project objects encoded as JSON. It wouldn't be keyed by source, though.
// We'd render the projects essentially the way we already do.

Likewise, when the user asks to install a single project, we'd do something like this (most likely in the ActionButton component):

import Installer from './Installer.js';

// This is a project object returned from the query manager, e.g. `<Project data=${dataForSingleProject} />`.
export let data;

// The installer knows what to do here. If the project is absent, it invokes Package Manager. If it's present, it does the activation.
// If it can't do Package Manager, it shows the commands. That's all handled by the Installer. The Installer also knows about the "add-to-cart"
// threshold, and reacts appropriately. None of that should be handled by the Project component or the button itself.
<button onClick=${Installer.install(data)}>Install</button>

Let's talk about the tests for a minute. The tests are very messy and hard to follow. They are duplicative in some places. They can be flaky because our PHPUnit testing API (not PHPUnit itself) isn't very good at waiting for things to be in the state we need to be them in. A few ideas on how to improve this:

• Find and remove duplicate test coverage.
• Convert the Nightwatch tests to functional JS PHPUnit tests. Core is abandoning Nightwatch, in favor of Playwright (probably), but that's not ready yet. In the meantime, nobody can figure out how to run Nightwatch tests locally, and most of our functional JS tests are already written in PHP. There's just not a lot of benefit to Nightwatch for us right now.
• Rewrite \Drupal\Tests\project_browser\FunctionalJavascript\ProjectBrowserUiTest::testMultiplePlugins(). This test is testing something valuable (namely, that user-entered filters don't leak where they shouldn't), but it goes about it in an incredibly awkward, nearly impossible-to-follow way that is very tied to the idea of multiple tabs in a single-page app. It's irredeemable, but should not be removed; merely rebuilt.
• Remove the RandomData source plugin. Using random data in tests is generally a mistake unless you need a random value. That's not what we're doing. RandomData generates random context (i.e., random categories and such). That's a big no-no in testing. Any tests using this source should be refactored to not use it, and the source itself should be removed.
• Add and use helper methods (maybe a trait) for interacting with Project Browser's UI in a way that respects the semantics of Project Browser (like, $this->filterByCategory('Foo') instead of clicking some extremely complicated CSS selector) and is aware of the asynchronous wait-for-it nature of functional JS tests.

I know this is a very long comment, and I hope it makes sense. This is my medium-term vision for a Project Browser that really fulfills our needs, works nimbly and impressively, and is both testable and maintainable. This issue takes the first step, let's take the next ones together.

At least this bug - hopefully it's not a can of worms:

Start on the "Browse" (modules) tab. Pick a category (ex.g. "Access Control"). Click to the Recipes tab. Recipes shows a flash with all of the included filters, then hides the filters, leaving recipes to show 0 results. (Full page reload doesn't help. Must be session storage.)

(Of note is that doing this and switching over to the random data plugin also shows 1 category there, but 'undefined' since I'm sure the actual category doesn't match.)

Hopefully there's a baby-step fix here but I think the separate nature of these changes where they're on separate tabs makes sense to really decouple ANY kind of sharing between filters. Even if they have the same filters, don't carry them over - make the user pick. Even if keyword is shared between the two, it now makes more sense to really decouple all those... each backend plugin gets its own and never the twain shall meet.

Hopefully, like I said, there's a lightweight approach we can take here, initially, for the benefit of unblocking Drupal CMS. Or maybe ripping out anything trying to do this IS the easiest fix.

Same on 11.0.9 and 11.1.0.

Sadly, it seems to be a bit of a rabbit hole. The Tabs logic is tangled up with the filtering in a really tricky way.

But I found a workaround: just hard-code display: none on the tabs component. This allows the logic to keep working as before, and we can factor it out later. But it gives us the UX we want, which is that the project browser is segmented by local tasks.

On second thought, I see what I missed -- you were right, @chrisfromredfin, it was holding on to stale crap from sessionStorage. I have now changed it to just unconditionally deleted whatever garbage is in sessionStorage, since we can't guarantee it will apply to this source anyway.

This one really turned into a dragon.

It turns out that much of the janky behavior that we've seen for a while is due, ultimately, to the fact that persistence layer (i.e., the stuff in sessionStorage) is designed under the strong assumption that every source will have the same set of filters. That's not true and it hasn't been for a while, and it caused some extremely sketchy workarounds in the Svelte code (e.g., the window.location.reload() call if the active tab changes).

I discussed it with Chris, and we agreed that the persistence layer, as currently built, makes no goddamn sense. Its goose was probably cooked as soon as we realized that different sources need different filters. It should never have been allowed to progress to the broken kludge it is now, but hey...live and learn.

So what to do? Remove that persistence layer. Make Project Browser absolutely dumb as a doornail - if you navigate to another page, it will forget what page you were on, and what your filters were. Paradoxically, this seems to be a better user experience than the weirdness and bugginess caused by the misguided effort to persist data between tabs that aren't consistent with each other.

Rip out the persistence layer for now, and we can restore it later with a saner design (I'm biased towards what I proposed in #15, but I'm open to discussion). The persistence layer can be, should be, a nice convenience for users -- not a distorted foundation upon which the whole application is built.

Is this still implemented as in the video in #10? That is not what the issue summary explains?

This does imply a UX change. Previously, if you went to the "Browse" tab, you'd get a whole Project Browser with all sub-tabs visible. Now, you won't. Each tab exposed by a source will link you to a single-source Project Browser page. If you want to see the whole Project Browser, with the sub-tabs, you'll need to explicitly go to /admin/modules/browse.

In the video, I think the Browse tab alongside the Recipes is confusing. "Recipes" is also browsing stuff. Also the Uninstall alongside the recipes is confusing, as you can't uninstall recipes, but you can uninstall some of the things from some of the other tabs :) At least I think browse would need to be renamed if that stays as the "Add extension" screen or so.

I think we do agree that a larger overhaul of what is the Extend section is in order, but it would be good to arrive at a little improved middle ground from what is on the video IMHO for the meantime.

@chrisfromredfin and I discussed that. We agreed that local actions would be clearer, but we don't want to add more work to what is already a very complicated issue to sort out bad plumbing.

The short-term plan:

• In this issue, kill the Svelte in-app tabbing. The Drupal CMS designs call for the enabled sources to be local tasks, so implement it that way. (Once the sources are available to Drupal's menu hierarchy, it can use ECA to customize them further.)
• In a follow-up here in Project Browser, convert the local tasks into local actions under a single Browse tab. This is probably going to be a fairly easy lift once the main dragon is slain in this issue.

Acknowledging that we're really starting to shift slop around in here, it's still nonetheless a step forward.

We need to really take a cleanup pass in here, per Adam's thoughts in #15. We will do that in follow-ups. The good news is this will result in far SIMPLER and MORE MAINTAINABLE front-end code.