🇦🇺Australia @sime

It's your project! - however you like is fine. I didn't know if you changed it because you thought it was already fixed in 1.8.

Is this the issue about getting the contextual edit links to work? I recall we discussed it but I'm not sure if i need to create a new issue for it.

You will forward patch this to 1.8? Usualy it gets fixed in main and then backported?

Sorry about that.

I believe that people who want to start working on drupal core have to make some decisions about what sort of work they want to do and what workflows suit that. I therefore think .ddev/config.yaml is too easy as it assumes those decisions are inconsequential. have talked a lot with @rkoller about the difference between justafish/ddev-drupal-core-dev and joachim-n/drupal-core-development-project as the both enable similar outcomes but the work area is different.

BTW I've tried to write up some of this into the canvas of the #ddev-for-core-dev Slack channel, which is a space for people to talk about these decisions and workflows.

I think the flex points that DDEV is supplying to different developers with different workflows are important.

so that a contributor only has to type ddev start and start working? ddev start

For me this would replace:

ddev config --omit-containers=db --disable-settings-management
ddev start
ddev get justafish/ddev-drupal-core-dev
ddev restart

But for @quietone this replaces something different, I'm reading.

I think you misunderstand me, it looks like your demo is connected to OpenAI. Project Browser wouldn't do that out of the box.

Do any of these integrations you work with allow you to embed a "small gpt" and avoid connecting to commercial services?

@mindaugasd love the examples, but i think the API integration is just not going to happen within the actual Project Browser code base.

@chris - I intentionally tried to keep the title loose. There is a tendency for people to say "plug in to Open AI" but I don't think integration into a commercial service is the way to go and a lot of the drupal modules I've seen are basically doing just that. But it would be silly not to have a place to discuss ideas.

sime → created an issue.

sime → created an issue.

Oh nice!

I can't speak for the extra data fields, I would love to hear an example of using the updated date on a card (which is the default being used here) - it's always interesting to learn about different requirements.

This is all really the job of entity bundle classes, but fwiw i've changed the theme to do this to get the created time.

I needed to add (int) because getCreatedTime() returns timestamp as a string.

  * Pre-process for Date node field.
  */
 function _civictheme_preprocess_paragraph__node_field__date(array &$variables): void {
   if ($node) {
-    $variables['date'] = civictheme_format_datetime($node->getChangedTime(), 'civictheme_short_date');
-    $variables['date_iso'] = civictheme_format_datetime_iso($node->getChangedTime());
+    $variables['date'] = civictheme_format_datetime((int) $node->getCreatedTime(), 'civictheme_short_date');
+    $variables['date_iso'] = civictheme_format_datetime_iso((int) $node->getCreatedTime());
   }
 }

I'm not sure if the date should be selectable but it definitely shouldn't use the "changed date" for promo cards, it should use the date created.

sime → created an issue.

griffynh → credited sime → .

Great progress! I feel very supported on this itch :)

Still needs a test to test clearing the cache on PreApplyEvent

sime → changed the visibility of the branch 3451863-cache-validate-pm to hidden.

This is identical to previously committed. I assume i messed up a rebase.
https://git.drupalcode.org/project/project_browser/-/commit/7a04045d3759...

sime → changed the visibility of the branch 3455220-resolve-technical-debt to hidden.

sime → changed the visibility of the branch 3455220-resolve-technical-debt to hidden.

sime → changed the visibility of the branch 2.0.x to hidden.

Sorry it appears i migth ahve readded this in a rebase

What was removed
https://git.drupalcode.org/project/project_browser/-/merge_requests/517/...

But it's still there.
https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/tests/sr...

sime → created an issue.

Specifically the model i like about civictheme is the UI Kit implementation with Storybook. However since experience builder will use SDC isn't there an SDC native design system technique we would be using?

> @sime can you help with number 3 in comment #27 for the off-focus/onBlur in Svelte. We just want to close the dropdown on click outside. Thank you.

I had a look and I got close, but I'm not experienced enough in Svelte to do this.

sime → changed the visibility of the branch 3318817-change-catfilter to hidden.

Going to assume we're on 2.x

sime → created an issue.

sime → created an issue.

sime → created an issue.

sime → created an issue.

> Is it possible there is something else causing the performance issues? ... Project Browser is by no means unusable, even with the slow status checks.

I agree it's within the bounds of being ok in "normal" circumstances. This is off course true, since no one thought it was a problem... One thing that slows things down is slow file I/O - there seems to be a lot of checks of the file system. What do you think that will be like with WebAssembly?

> the issue summary says we're running the status check on every page

I found that it was running when I was looking at project detail pages, which is most of the routes you can explore - did you not find this?

Btw. my main goal so far was to demonstrate for myself (and prove to others) that the browsing expereince was significantly improved with caching. So I'm not really wedded to *how* the caching should work and I was mainly waiting to see how the message improvemnet issue landed because it was calling the validation check.

Ya know, I think this is RTBC. Feels solid to me. Will let Chris have the final say.

For anyone who wants to manually test this, a simple way is to install the project_browser_test module.

1. You need this in your settings.php, note that it will start showing test modules/themes in the UI.

$settings['extension_discovery_scan_tests'] = TRUE;

2. Then install project_browser_test, I just use drush.

3. Now in TestInstallerReadiness.php in the statusCheck method, just change the code to manual create whatever messages you want to create.

Re #2, InstallerControllerTest often calls routes like `admin/modules/project_browser/install-begin/drupalorg_mockapi/awesome_module` and this triggers status checks which can time out. I believe.

I analysed the events that PM provides using xhprof, and the StatusCheck event is by far the only event where significant amount of time is spent in our tests, which specifally is any tests where:

• Package Manager is installed
• drupalGet('admin/modules/browse')

.

This is the same as 📌 Too slow to validatePackageManager() on every page Needs review but obviously the solutions will differ. I've created an MR to try @phenaproxima's suggestion in Slack which is to put an event first and then stop propagation.

For the record these are the events subscriber that are running when the test his /admin/modules/browse where the vast majority of time is spent:

• Drupal\package_manager\Validator\ComposerValidator::validate
• Drupal\package_manager\Validator\DiskSpaceValidator::validate
• Drupal\package_manager\Validator\WritableFileSystemValidator::validate
• Drupal\package_manager\Validator\MultisiteValidator::validate
• Drupal\package_manager\Validator\SymlinkValidator::validate
• Drupal\package_manager\Validator\StageNotInActiveValidator::validate
• Drupal\package_manager\Validator\BaseRequirementsFulfilledValidator::validate
• Drupal\package_manager\Validator\PendingUpdatesValidator::validate
• Drupal\package_manager\Validator\LockFileValidator::validate
• Drupal\package_manager\Validator\AllowedScaffoldPackagesValidator::validate
• Drupal\package_manager\Validator\StagedDBUpdateValidator::checkForStagedDatabaseUpdates
• Drupal\package_manager\PathExcluder\UnknownPathExcluder::logExcludedPaths
• Drupal\package_manager\Validator\SettingsValidator::validate
• Drupal\package_manager\Validator\RsyncValidator::validate
• Drupal\package_manager\Validator\ComposerPluginsValidator::validate
• Drupal\package_manager\Validator\ComposerPatchesValidator::validate
• Drupal\package_manager\Validator\PhpExtensionsValidator::validateXdebug
• Drupal\package_manager\Validator\PhpExtensionsValidator::validateOpenSsl

Changes I made result in this:

The worst case scenario is two messages of the same type, thus showing the same prefix, but I think this is acceptable as it's very rare and looks fine.

I'm just going to combined these up a little. It would be rare to have multiple errors here, I believe, so we'll just prefix each one and skip the extra ones?

Thanks @phenaproxima! Satisfying nit.

Everything checks out :thumbsup:

Thanks for the guidance!

sime → changed the visibility of the branch 3322594-DrupalConPortland2024-snapshot-1x to hidden.

sime → changed the visibility of the branch 3322594-DrupalConPortland2024-snapshot-1x to hidden.

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

@phenaproxima has tightened the scope and it is looking good to me. I like the improvements in variable handling.

I have created a few extra tickets out of the things I learnt in the course of working on it.

📌 Resolve technical debt in initPackageManager() Active
📌 ServiceProviderBase::alter shouldn't call parent::register Active
📌 Identify and action solutions for flaky tests Active

Before i forget, Chris wanted to be sure that this code was aligned with how AU/PM handles caching of this same data. That was going to be something I looked at after 📌 Do not prevent UI install for PM warnings Needs review .

And it would be so easy to swap KV with something else if needed later.

Everything else makes sense and it's worth a maintainer having a look - the key thing I see is changes to how the projects are uniquely identified and then all the services are adjusted to support that.

sime → created an issue.

sime → created an issue.

sime → created an issue.

If I'm reading the code correctly, every unique $SOURCE_PLUGIN_ID/$SOME_LOCAL_ID gets a separate key set? If this numbered into the thousands or tens of thousands I don't this kv is the best place, especially being the place I most associate with Redis, and these IDs are not values I assocaited with require super fast runtime read/write. Would it be better to use a cache bin?

> Also there was a discussion of allowing the uninstall of recipes after installing and playing around with them if you haven’t changed any of the configuration for them.

Yeah, "apply a recipe" is the language - I note that the Drush command has this language. Obviously "unapplying a recipe" is a compelling idea but being able to save a snapshot of the site (eg using workspaces suggested here) before you applied a bunch of recipes is IMO how starshot could be looking at it.

Just re-summarising this progress. The previous progress is still here against 1.x.

Testing strategy

Compared to the 1.x progress, I changed the way errors were mocked so that test logic did not need to go deep into Package Manager. So the updated tests are testing scenarios InstallReadiness generates errors, rather than testing how PM generates errors.

The reason i changed the tests was because it was very flaky (intermittent time outs and failures) and slow for me (even without xdebug). I think this remains a bigger problem with a lot of PB tests seem to do full integration not always for obvious reasons. In these case we are flexing the PM code just to see how ProjectBrowser.svelte handles these messages.

Long story short, I can revert how the tests are architected if asked. it was a journey for me to get them to work but i'm not wedded to my approach.

Message strategy

Instead of just setting status messages in PHP, we are turning them into dumb strings and passing them to the svelte app, which then uses the javascript equivalent of setting the messages. We could just set them in PHP. Basically the current 2.x code is still doing the same thing as a the 1.x coee, I just wanted to point that out.

Couple other points:

1. I did change the svelte app so that it clears and current status messages just to avoid the screen getting filled with messages.
2. I aligned variable names so that everything from package_manager is prefixed as such.

Installing test dependencies

A big thing in this MR is that it seems like we no longer need to install package_manager in a weird way via initPackageManager() and we can just use the $modules property. That seems like a good bit of technical debt removed.

I started to fix the tests but the only phpunit tests that are breaking at the FunctionalJavascript issues that seem to be mostly about the UI changes, and I wasn't 100% if they are in flux. Otherwise the 2.0 MR is good and I'll keep it up to date with 1.0 changes if we're doing both.

I've created a branch 3318817-change-catfilter-for-2x to prepare the 2.0.x version of this branch in parallel. It's not clear if this will be committed to both 1.x and 2.x.

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

Updated to 2.0.x (new branch). Old branch kept.

I've preferred settings.php overrides for nearly 10 years, and they have felt like a second class citizen to contrib solutions for all that time. I support the sentiment of #267, #268, etc. Massive step forward.

I tested the same things as #272. Since I last looked at issue I see there's a twig template for the message which was a nice surprise! So I also tested that I could override that, because for now I would do that as a standard practice to explain to site builders where the values are being overridden.

I think there are good tasks here but it's a different landscape with starshot.

Non working state, removing from needs review.

I mostly resolved @fjgarlin's comments from 1.x MR

Rebased and created new PR on 2.0.x branch.

sime → changed the visibility of the branch 3365180-pm-warnings to hidden.

sime → created an issue.

Small change to phpstan.neon. I would like to keep reportUnmatchedIgnoredErrors: true so that we are warned whenever an ignore is no longer required. I also made the ignores more terse.

I'm just putting some phpstan in a different branch and playing with the best way to cover the Recipe stuff that also works for drupal 10.2. If this gets merged first I will just do a follow up issue.

I'm psyched about this, the code is tight!

I'll be proposing some phpstan simplification shortly, low key stuff.

Next positive reviewer, if no obvious issues, please RTBC to keep things rolling for the recipes work.

Reading the code, the changes make perfect sense to me, and the ActivateInterface is intuitive when you trace that through to ModuleActivator it's good DX. Obviously this is a pretty important ticket for abstracting things out and make installing recipes possible.

Big +1. Probably needs someone with longer experience with PB that me to RTBC.

That would allow us to get rid of the text below that says what to do if you don't have Drush.

Safe to keep it out of scope, but this is something that has niggled me for sure.

Subjectively, everything is good about this. I have discussed with @acbramley just to make sure i'm not misreading anything the emerging practice of autowiring. Loaded everything locally, I can't see any reason not to RTBC this.

The only note is that this technically this is breaking BC - assuming other contrib projects actually use PB services, they would have to update some service references, since we don't add aliases to the old service IDs. I think a clean break is ok on the 2.0 branch.

I still have this assigned to myself just because if someone wants to pick it up it would be great to talk through the approach.

Some thoughts on next steps

1. Review the 6 month old unresolved comments
2. I think we should change the wording "messages" to "warnings" the same way that "errors" => "errors"
3. I think I have a plan for removing the need to isntall package manager, thus hopefully further reducing flakiness