- Issue created by @jnicola
- πΊπΈUnited States phenaproxima Massachusetts
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.
- πΊπΈUnited States jnicola
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.
- Merge request !8012Expand RecipeDiscovery to return a list of recipes from recipe directories - Issue # 3446354 β (Closed) created by jnicola
- Issue was unassigned.
- Status changed to Needs review
about 2 months ago 2:47pm 10 May 2024 - πΊπΈUnited States phenaproxima Massachusetts
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, andcore/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.
- Status changed to Needs work
about 2 months ago 2:52pm 10 May 2024 - πΊπΈUnited States jnicola
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.
- Merge request !8023Issue #3439909 by SolimanHarkas, vensires: Fix Taxonomy tests that rely on... β (Open) created by jnicola
- πΊπΈUnited States phenaproxima Massachusetts
A few small points, but in general this makes sense to me!
It will, however, need explicit test coverage.
- π¬π§United Kingdom alexpott πͺπΊπ
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.
- πΊπΈUnited States jnicola
@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.
- πΊπΈUnited States thejimbirch Cape Cod, Massachusetts
"resolved all threads" - is this ready for "Needs review"?
- πΊπΈUnited States jnicola
@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!