Create a simple class to allow discovering recipes in the file system

Personally, I wouldn't follow ExtensionDiscovery's API here, purely because recipes aren't extensions and shouldn't be conceptualized or treated as such. But the ability to find them (to be exposed in Project Browser or other UIs) does seem handy.

We'll need to consult carefully with @alexpott on this one to be sure we don't introduce regressions, because the recipe system is very sensitive to changes in discovery.

Some of us have been talking about it at length over in slack. We're also chatting about it as a group at the Drupalcon2024 general contribution Recipes table.

The decision appears to be that a discovery class would be useful. There are restrictions around paths. This also does not exactly copy ExtensionDiscovery. It just follows the method naming conventions from there.

This currently affects nothing as far as Recipes per our local testing and none of the code already in discovery (a single method) is influenced by this addition.

The discovery class can now return an array of Recipe objects as available in core/recipes and /recipes.

This can be used by layout builder or any other future effort that should desire to acquire a list of all recipes that can be loaded as Recipe Objects, and should help prevent redundant code from coming to exist such as in a plugin in Project Browser and then onwards in other places wishing to display a list of recipes.

Ready for review by others.

This is very similar to what RecipeDiscovery used to do, but was reverted.

We probably want to adjust this slightly. If I understand the design intention correctly, are only two places that it should look -- the arbitrary $path that's passed to the constructor, and core/recipes. That's it. In other words, it should not hard-code to look in /recipes.

Which is what it already does in HEAD, to be clear -- but what we need here is an additional method that shows all recipes in those two paths.

So I don't think the $path to constructor idea is wise, but left it since it's not my work.

If anything, that should be removed.

Otherwise, people can specify any path they want to search... which is exactly what you specified we don't want to be doing.

Also, ExtensionDiscovery is explicit about paths, it's not unreasonable to be explicit here.

I can alter the constructor and chase down all code using this as is, but I chose to alter as little other code as possible.

I think a follow up issue to tackle that would be reasonable to keep this issue scope to just the ability to get a scan of all discoveries in the directories Recipes currently intends to restrict users to.

jnicola → closed merge request !8012

A few small points, but in general this makes sense to me!

It will, however, need explicit test coverage.

I've opened 📌 Move RecipeDiscovery into RecipeConfigurator Needs review to remove the existing RecipeDiscovery. I'm not sure we need this work yet in core but at least there will less confusion about the purpose of the class that was called RecipeDiscovery.

@alexpott I believe the linked Project Browser work will need it as it needs a way to display all the available local recipes.

We also developed a recipe_ui module at Drupalcon that will also need to discover recipes. The intention was to put it in core, but since the Project Browser push is the main focus, it would be a contrib module... if we even push out what we built at all.

In general though being able to discover recipes seems like a useful chunk of functionality for folks so as everyone isn't reinventing the wheel.

Moving to the new recipe component!

"resolved all threads" - is this ready for "Needs review"?

@thejimbirch it's sort of ready!

Since I resolved everything, RecipeDiscovery was moved to RecipeConfigurator (see https://www.drupal.org/project/drupal/issues/3447210 📌 Move RecipeDiscovery into RecipeConfigurator Needs review ) so I had to redo the work on RecipeDiscovery. So I redid the work, added some functionality based on some suggestions from @phenaproxima and had pinged them for a code review. I'd love for you to take a gander at it as well!

I also need to add tests. Writing those would be the goal early this week, but I am having trouble getting DDev to run phpunit tests. I couldn't even get ddev to spin up Drupal 11 without some notable deviation from the main Ddev directions, so I'm sure the problem lies somewhere in thee.

If anyone reading this has availability to pair through writing Phpunit tests with me, I'd be forever grateful!

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Ah okay, got it!

I've got time at work and am going to try and get some tests for the updated recipe discovery class in the MR and then see about getting this out.

This has been updated and now has tests that check the RecipeDiscovery class works as expected.

Wouldn't mind some additional review from folks.

jnicola → changed the visibility of the branch 11.x to hidden.

jnicola → changed the visibility of the branch 3446354-expand-discovery-class to hidden.

Any recommended way to test this one?

One simple idea how to test: You could write a custom controller that calls RecipeDiscovery and gets all of the recipes from core and outputs them in a nice list, or a table, or whatever makes you happy! If it finds all of the recipes as expected, that's good! If you want to see actual output, do a list output of each recipe returned to you!

You can also test this works by attempting to load the test recipes. where it will load all but one that shouldn't load because it throws an exception.

Would love it if some more folks got interested. I'm still flummoxed that a standardized way for folks to discover Recipes is an uphill battle with outright disinterest. It's open source, give people the tools and let it grow!

Well, to be clear...the way to "discover" recipes is, at least for now, going to be Project Browser, which implemented its own mechanism.

Having said that, a core mechanism would be preferable. I could see a use case where you might have several distinct "cookbooks" in a given code base, and having the ability to browse each of them using a standard class supplied by core might be beneficial.

@jnicola and I had extensively negotiated the parameters of this feature, and we are respecting an intentional limitation of the recipe system, which is that recipes can only be scanned for in two places at once: an arbitrary directory (normally wherever Composer is configured to install packages of type drupal-recipe, which in most sites is probably /path/to/project/recipes or /path/to/project/webroot/recipes), and the core recipes directory.

So this MR, and the class in it, could maybe be more precisely named; "RecipeDiscovery" sounds a lot like RecipeDiscovery, and seems to imply that it will find recipes anywhere in the code base, which is not the case. We can bikeshed the naming later. As it is, I'll review this with the naming set aside.

I took a look through this and I think it's a good start but needs to be greatly simplified in some spots. But hey - there's test coverage, and that's great!

I would strongly recommend looking at what Project Browser did, and borrowing heavily from there.

Well, a few things to that end.

Yes, Project Browser is the way folks will likely find recipes. That said, as with most things in Drupal, the "next big thing" historically comes and goes, but it's the base concept that remains. That's the beauty of it all being so extensible, and has allowed Drupal to have reinvented many aspect of module management, config management, content creation, layout management. From panels, to beans, to features... can we say someone won't come along with something better than Project Browser in the future? I don't think staying true to Drupal's history we can.

Respecting the desire though to give Project Browser it's fair shake, while also recognizing extensibility is the ladder that got us all on this roof... I kept RecipeDiscovery built in such a way that while it heavily favors the two directories you suggest we try and focus RecipeDiscovery down to... it can be used to discover recipes in other locations.

So if install profiles ever make a comeback and ship with Recipes, or hey maybe the next big "starshot" will have it's own directory... whatever the next big thing is, this can accomadate for it, but favors core, just like ModuleDiscovery.

Ultimately, I think this threads the needle of being as true to the original ModuleDiscovery object as possible, while navigating the current desired direction of Drupal.

As for needing more work, could you please be specific so as this can meaningfully stay moving? Just saying "Do better, see this" leaves this perpetually in limbo. Plus, I did review your project browser and did borrow from some of it.

I didn't want to give the impression that I have a problem with the way this code has been designed. I agree with the design - it favors finding recipes in core, and an arbitrary place. That is what we had agreed on.

I'm asking for changes in certain specific places - the way the code is now is doing a number of questionable things (the logging, for example; the reliance on scanning for paths instead of just using Recipe objects is also a bit of a head-scratcher). The design is sound; it's the implementation needs some streamlining, fine-tuning, and clean-up.

(The naming probably also needs some work to conceptually separate it from modules and other types of extensions, but that's a separate conversation, and it's easy to resolve.)

Ah, I am not seeing them for some reason. I'll go look again and see what I can address from them, thanks for your time as always!

I think this pretty much covers it. This will need review/RTBC from someone else; @jnicola and I both worked heavily on this patch.

Looks good. Marking as RTBC, thanks!

I don't think we should be adding user and system to recipes. Adding required modules to this feels really odd. Can't we just install the modules in the kernel test and be done?

Re #41: in that case, we will need a blocking issue to fix the validation, which currently disregards already-installed modules when considering the config entity prefixes listed in the actions section of recipe.yml.

Opened 🐛 Recipe validation should consider modules that are available, not just the ones listed in recipe.yml Active as a blocker.

Blocker was committed!

Extremely small fix, otherwise looks good to go

Appears to have a test failure.

Some may be solved by a rebase since the MR is 156 commits back.

But this one caught my eye Drupal\FunctionalTests\Core\Recipe\RecipeCommandTest::testExceptionOnRollback if caused by the change to core/tests/fixtures/recipes/config_rollback_exception/recipe.yml