🇺🇸United States @phenaproxima

This could use a test, I think. Can you add one to drupal_cms_content_type_base's ComponentValidationTest which:

• Adds a draft node of any type (you could apply drupal_cms_page to provide one to test with)
• Logs in as a content_editor
• Visits the moderated content page and confirms that the node is visible (link should be there)

As of about 5 hours ago (the daily overnight test run), the performance test was passing in HEAD. https://git.drupalcode.org/project/drupal_cms/-/pipelines/375659

#37: Resistance? Who's resisting?

The performance test is beneficial, but I insisted on adding it in a separate allowed-to-fail CI job for a few reasons:

• I myself don't fully understand how the performance tests work or how to properly fix them when they fail. It's not that I don't want to, but I simply do not have time for it, in light of all the work that needs to get done for Drupal CMS's stable release in January.
• We have a large number of dependencies that are not pinned, and that we don't control. Any one of them could make a change at any time which would break the expectations of the performance tests. This is a completely different situation from core, which has far greater control.
• Many of our contributors don't seem to be super comfortable with automated testing at all, let alone performance testing for which I can't yet offer any guidance. Seen this way, adding a potential new barrier to contribution would be counterproductive, slow velocity, and make more work for me.
• This is just my opinion, but I think performance is not going to be a priority for our target audience of marketers. It's just not. There are plenty of site owners out there who will gladly bog down their websites with gigabytes of JavaScript in order to make users have a particular experience, with no concern for performance. (It's exasperating, but it's the reality.) The fact that we are being more responsible than that, and trying to make Drupal CMS snappy, reflects well on us...but it need not be a priority.

So yes, I embrace the performance testing. I think it is a good thing. But it cannot become a barrier to our rapid iterating.

Merged into 1.x. Thanks!

Crediting @heddn for some behind-the-scenes mentoring work. :)

Magnificent - thank you! Will merge when tests pass.

If you also don't see a "Create merge request" button in GitLab, maybe try pushing a differently-named branch to this issue fork?

In theory the branch name shouldn't matter, though, so I'm not sure what's going on.

You don't need to run the tests locally. They are will automatically run if you create a merge request in the GitLab UI.

(Also, please do not delete the issue summary when you post comments. :) Maybe refresh the page before posting your next comment, just so that any emptied form fields are refreshed?)

That looks good, but you need to open a merge request so that the tests run and I have something to merge! :)

Here's some (admittedly not very good) documentation on how to do that, if you're unfamiliar with the process: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → . I tried to do it just now but it didn't work for some reason.

The branch doesn't look correct yet, though. Can you please implement what @jurgenhaas asked for in #2, which calls for removing the dependency on Gin Toolbar, and only relying on Gin?

As per Jurgen’s comment, the Gin Toolbar dependency should be removed. Looks right to me otherwise.

The Nightwatch failures are preexisting; not your fault.

I think that doing a skip if the source is recipes is a completely legitimate workaround.

I kind of wonder, though, if it would be super hard to move the install readiness checks to \Drupal\project_browser\Controller\InstallerController::begin(). Because that is the only time Package Manager will ever be invoked. The only thing the Svelte app would need to know is whether Package Manager is present at all. It wouldn't have to care about the validation results or anything like that.

Maybe we can try the deeper fix in another branch of this issue fork, and then commit the quick fix if it turns out to be a bugaboo? Or, hell, we could commit the quick fix but keep this issue open and work on the deeper fix in another branch.

In either case, though, this branch needs to be synced against 2.0.x HEAD due to merge conflicts. :(

📌 Get coffee data only when the search box is opened Active got in.

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.

Merged into 1.x. Thanks!

Looks like this needs to be synced with HEAD.

This will need test coverage to prevent it from regressing.

phenaproxima → created an issue.

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

This now has strong test coverage, against the drupal_cms_analytics recipe itself (which is now a dev dependency).

I think it's ready for review.

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.

Klaro tagged a new release and performance tests are now (expectedly, beautifully) failing in HEAD: https://git.drupalcode.org/project/drupal_cms/-/jobs/3776271

Let's get this done!

phenaproxima → created an issue. See original summary → .

phenaproxima → created an issue.

Merged into 1.x. Thanks!

The exception in #12, it turns out, is due to a core bug: 🐛 Recipes that depend on other recipes break RecipeInputFormTrait Active

The good news is, we can work around it.

phenaproxima → created an issue.

phenaproxima → created an issue.

This isn't really ready for any changes yet. :)

Assigning to Jim for input.

The sitemap is also part of our SEO offerings, rather than any one content type, so switching track.

Directly committed this without a merge request. It'd be extra overhead to correct what was clearly some kind of snafu during the RC2 release process.

Ah - I see what happened. The installer cache was still present in HEAD of 1.0.x, with outdated permissions!

We no longer have the cache in dev branches (only tagged releases), so deleting that should make this go away.

phenaproxima → created an issue.

That's a good start! But, I would like the tests to be approached a little differently. Right now they're testing the menu_ui config directly, which isn't very useful and doesn't make a ton of sense. After all, we know what the config will be; we can read the YAML files for ourselves. What we want to test is: will the config have the desired effect? Will users have the experience we want them to have?

In other words, what the tests should do:

• Log in as a content editor
• Visit the /node/add/WHATEVER page
• Confirm that the menu settings don't show up

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

Turns out HEAD in this branch is failing even with the reverted commit. So, reverting the revert. (See https://git.drupalcode.org/project/drupal_cms/-/jobs/3768287#L78)

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.

This broke HEAD in 1.0.x...

Merged into 1.x. Thanks!

kristen pol → credited phenaproxima → .

My response on Slack:

So you cannot test Package Manager in the context of our DDEV project. It won’t work for several reasons that I’m too tired to explain right now. Long story short, you gotta spin up a “real” project to do it.
composer create-project drupal/cms:1.x-dev

Does that get you any further?

Merged into 1.x. Thanks!

phenaproxima → created an issue.

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.

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.

Merged into 1.x. Thanks!

Merged into 1.x. Thanks!

Good to go if it passes tests!

Merged into 1.x. Thanks!

Oh come on...you barely wrote any CSS. I was expecting an entire inscrutable style sheet. Didn't even get that!

I agree - we need to start landing these, desperate times and such. Better to do it with some rough edges we can smooth out later than not do it at all.

phenaproxima → made their first commit to this issue’s fork.

Transferring credit from 🐛 The "dashboard" parameter was not converted for the path Needs work , which appears to be the same bug in a different form.

The other issue is in, so on the latest 1.x...does this still happen for you?

If not, let's mark this one closed as a duplicate. :)

This is a very tricky bug that I wasn't going to have the bandwidth to dive into before general release, but man, the community really stepped up. I'm grateful for all your diligent reporting, testing, and work in this issue.

Joyfully merged into 1.x. Thanks!

That MR looks straightforward and although the commit is blocked on getting a new tagged release of Klaro, I see nothing to complain about here.

So this is (probably) causing two bugs, eh!

That, in my book, makes this a critical.

It worked for me so I have pushed that to the MR. If any of you lovely people would care to test it out, I'll happily commit this.

I reproduced the same problem, @rushikesh raval. Changing it to \Drupal::service('kernel')->invalidateContainer(); fixed it (my bad!), and the installer succeeded. Try that?

There have been some really interesting developments over in 🐛 Running the installer leads into a wsod Active . Our current prime suspect (hah!) is a stale container hanging around in the cache.

I'd say let's keep this bug postponed on that one. When that one is fixed, we'll see if this problem persists.

There's debugging, and then there's debugging. #30 is the latter. Wow!

Any stray request handled by the default front controller and DrupalKernel during installation will trigger a container build / dump.

@rkoller mentioned to me in Slack that he saw in the server logs that during installation, Safari -- and Safari alone, presumably -- is making requests for some icons (I can't remember their names) that are not existing in the project, thus triggering 404s. Those are probably the stray requests that mess up the container, and would also explain why this is only affecting Safari. What's more, it would explain why this is only affecting certain versions of Safari.

I wonder if doing an explicit container cache clear in drupal_cms_installer_uninstall_myself() would fix it. Could someone give that a try? Just add this as the last line of the function:

\Drupal::cache('container')->deleteAll();

Let's give that a try and see if it squashes this bug.

One question.

Looks like this needs to be synced with 1.x?

@pameeela, the problem is likely the fact that e2e tests apply against the Standard profile, which ships the contact form, and our recipe does not clobber it.

Merged into 1.x, thanks for the review and tweak!

If you like it, in it goes.

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

as you just removed what looked an practical tool for easily generate a Drupal Recipe,
is it out there any handy way / tool, or even instructions on how to to easily generate a Drupal Recipe, from a working Drupal site?

It was not, and never was, a practical tool to generate a recipe. It was only meant to be a convenience layer to add a new recipe stub to Drupal CMS itself. As I said -- it was for contributors. It wasn't well documented, because it was, well, internal! :)

It doesn't generate a "recipe", per se - all it did is generate is a composer.json file (if you read the script from the git history, you'll see that for yourself). Recipes themselves still need to be constructed by hand. I'm not aware of any tool to generate a recipe from an extant Drupal site, but I know a lot of people are interested in that. I suspect it's on the Recipes Initiative's roadmap, probably phase 3.

So long, scripts! Merged into 1.x.

Oh, one other thing:

referring to this official instructions

Those instructions are not official! Nothing in the wiki constitutes official documentation. I'll update it to explicitly say that.

I'm just gonna remove them.

Hah, whoops. Thanks for that.

ddev recipe-create isn't meant to be used by consumers of Drupal CMS, though; it's just for internal development. Self-assigning to fix this.

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.

phenaproxima → created an issue.

Assigning to @ckrina for input.

Yes! There is a whole documentation track for Drupal CMS. It is being led by Amber Matz → and eojthebrave → . I would strongly suggest reaching out to them in Slack if you would like to help - I'm pretty sure they will welcome any assistance. :)

a site is only allowing stable releases

To be clear -- the site is only allowing stable releases in the absence of a stability flag to override that. Project Browser can install anything it wants regardless of the preferences in composer.json if it makes calls like $installer->require(['drupal/webform:@alpha']);.

Looking at the backtrace again, you can see one or two frames further down in the call stack:

Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber->on404()

So a 404 error (presumably) is occurring, somewhere, somehow, and we're getting another failure while trying to handle that, because /system/404 isn't an installer page, so $install_state doesn't exist, but the profile is still installed (and therefore its library_info_alter hook runs).

That's why the early return is probably the "correct" fix, but it would be nice to know what exactly is triggering a 404. That may be where we'll find out why this only affects Safari.

However, speaking of due diligence. As I scanned through the backtrace in the issue summary, this one line jumped out at me:

Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase->onException() (Line: 246)

So an exception happened somewhere, and that has triggered a subrequest for (presumably) the exception page! The subrequest doesn't have $install_state available to it (that only ever happens during the install process), so it craps out.

So although I'm in favor of just papering over the WSOD by adding an early return in drupal_cms_installer_library_info_alter(), we should also figure out where the original exception is coming from, and what the problem is, and fix that. I'm guessing that adding the early return will actually enable us to do that. Can someone open an MR, please, which adds the early return, then try again and let me know what the new result is?