#Drupal CMS stable release blocker

I created an issue in Dashboard already, should have commented.

Let's move this to dashboard, should hopefully just be a permissions check before changing the redirect. Then we can see how that affects Drupal CMS specifically and whether auth users need a custom redirect or not.

I agree this needs to be fixed in Dashboard, it will be a problem on all sites, not just Drupal CMS.

Giving authenticated users access to the dashboard might be an issue as this is an admin page, uses the admin theme, and not every authenticated user may have access to the admin theme. As a result, the dashboard may look awkward.

We discussed more fine-grained redirects depending upon role and/or permission before. But the dashboard module has the redirect hard-coded, and we can't make any changes, unless they agree to either remove that redirect or make it optional so that we could come up with smart redirects by turning the fixed redirect off.

Discussed #8 with @phenaproxima we agreed that in this situation the event is offering benefits - the event object collects skips and validates them. This type of collecting and validating with an object has not been done with hooks yet and there is no need for this to be first.

Also discussed with @longwave ahile back and we agreed to backport this to 11.1.x because while this is not a bug in core it helps fix a bug possible when you have trash installed - which Drupal CMS does.

Committed and pushed 51ae03891c1 to 11.x and c813f52a5be to 11.1.x. Thanks!

And it'll need test coverage.

I agree that this is both critical and stable-blocking.

I don't think there's any real harm in backporting it, but also not much value (yet) beyond keeping it consistent, since RecipeInputFormTrait only appeared in 11.1.

Committed and pushed ac347da742f to 11.x and 379ebd46fcc to 11.1.x. Thanks!

We could backport this 10.4.x and 10.5.x if we choose.

This makes a lot of sense, some real fun nuance in there with

-  private ?array $values = NULL;
+  private array $values = [];

:D

Project Browser can work around this problem (by prying open the static cache with reflection and clearing it), so this is not a stable blocker for Drupal CMS.

But it is a legitimate bug that needs to be fixed. :)

Tests are failing as expected: https://git.drupalcode.org/issue/drupal-3495305/-/jobs/3776754#L54

This is ready for review and has failing test coverage.

phenaproxima → changed the visibility of the branch 3495305-test-only to hidden.

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.

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

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.

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.

I have created one follow-up, which seems more serious than the is-active class: 🐛 Cannot access Project Browser UI for contrib modules listing when on Recommended tab Active

I simplified a bit some of the cosmetic changes. I haven't tested anything yet so I don't know how close we are to a solution.

It's defo a good beginning, I made it into an MR so we can continue there, give feedback, etc.
https://git.drupalcode.org/project/project_browser/-/merge_requests/651/...

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.

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.

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.

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.

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

Attached is a patch that I started. Not entirely sure I'm on the right path. Would love some feedback and/or it can help someone else as a starting point.

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

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

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!

This isn't a stable blocker for Drupal CMS.

Not a stable blocker for Drupal CMS.

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.

Looks good to me, thanks @phenaproxima! Left a comment on the MR, feel free to close it if you disagree :)

And, done.

Hmmm...yeah, okay @amateescu, you've convinced me that we should support filing a reason why the skip happened (the reason should be optional). Trash's use case makes perfect sense.

Trash could use that to say that an entity was skipped because one with the same UUID is in the trash bin.

I would advocate to do that in a follow-up.

Hi folks, great work on getting this fixed. I'm using this successfully in my development environments along with the patch in ✨ Add a Normalizer and Denormalizer to support Layout Builder RTBC to propagate layouts.

I've run into an issue that may or not be related. If the original content type for which the layout builder is enabled is imported into a new environment, the layout builder setting is ignored. So I find myself having to manually enable the layout builder across environments. Once I do, then the import process above will work and I can see my custom layouts. Is this a bug perhaps?

I'll deal with this.

Marking as RTBC. Love to see some more improvement in the default content system!

Went ahead and implemented the skip mechanism.

Yes, these are known issues to handle in follow ups, just have not gotten around to creating them!

I think there are two issues here (not sure if I should create a new issue for this, so commenting for now):

1. The proposed solution was to set this as a default page when user click on "Extend". This seems not done
2. When you open the "Recommended" tab, you have "is-active" class on both "Recommended" and "Browse" tabs, which is not ideal

Thanks!

This is not a Drupal CMS stable blocker. It would be very nice to have it, and hopefully it can land in a patch release of 11.1.x as a change in an experimental subsystem, but it does not block a stable release of Drupal CMS. Getting this would, however, allow us to avoid a nasty bug that is relatively edge-casey, so keeping it as a release target in case this lands in a core patch release before Drupal CMS's general launch.

I agree - a good improvement by itself, and certainly worth iterating on more. Merged into 1.x. Thanks!

Tests are failing; I think you need to change the assertion of a "Recipes" link to "Recommended".

Suuuuuper excited to see this working! I made two minor changes to align with the designs: Updated the menu link to be 'Recommended' (we want to avoid using 'Recipes' in this context since it's pretty meaningless to Drupal newbies) and moved it to be first.

Still some work to do but marking RTBC because I think this is great as an incremental improvement.

I discussed this with @tim.plunkett and @pameeela and we decided it is not a Drupal CMS stable blocker. It's a nasty bug, but likely an edge case. And besides, the fix depends on an upstream change in core that is not itself a Drupal CMS stable blocker. So by extension, this isn't either. I'm downgrading it to a release target, as it would be nice to get it fixed if core can get the requisite change into 11.1.2 (and hopefully that gets tagged before we launch 1.0.0).

This should be in the default content system, not the recipe system.

Happy with this. Assigning to @pameeela for final review.

Added test coverage, which is failing because the Recipes tab doesn't get the is-active class. Any insight into that, @jurgenhaas?

Well that is damn nice. I'll write a test and sync it up with HEAD.

Gotcha, sure I'm fine with the idea.

@smustgrave it won't help. The problem is that composer can't load the required library without the declaration of an extra repository. And that needs to be done manually in the root composer.json file, i.e. that's manual work and therefore not a solution for Drupal CMS.

As composer already knows about the repository of packagist.org, we can easily make the libraries available there and every composer can require them without any extra or manual setup. In addition to that, this approach also solves the need that the libraries then get installed in the right directory as well, i.e. /web/libraries. Also without any extra configuration.

Would it be easier to add a composer.libraries file?

Changing tag to target since this is a pretty last-minute request and is not trivial.

Thanks @pameeela for introducing this.

Happy to help out with this, if maintainers agree to proceed with this. Here is what we've done in other places like Fullcallendar → or Klaro → :

• Create a general project on d.o as a mirror of the remote library
• Add a composer.json to that repository, set the name to drupal/NAMEOFLIB and set the type to drupal-library, also add the installer-name in the extra section to the name of the directory into which this library needs to be installed
• Push that code and create a release, ideally with the same version number as the referencing upstream library

The packaging system from d.o will publish that project to Packagist and you can now add drupal/NAMEOFLIB to require section of the composer.json of this project. From now on, the library will be installed automatically without the user having to do anything manually.