Expand Drupal\Core\Recipe\RecipeDiscovery to allow discovering available recipes, likely for use in Project Browser

Created on 9 May 2024, about 2 months ago
Updated 21 May 2024, about 1 month ago

Problem/Motivation

The discovery object for recipes is currently limited in scope. Expanding the class to discover recipes only in the currently intended folders will ensure contrib modules or project browser (when it moves to core) won't have to reinvent the discovery process.

Proposed resolution

The RecipeDiscovery will have similar method names to ExtensionDiscovery to help standardize the code.
The discovery object will have a method that returns an array of Recipe objects, as discovered.
Paths and path structure for recipes will currently be quite limited to just core and /recipes, intentionally, but can be easily altered to include other directories if so desired.

User interface changes

NONE

Release notes snippet:

(TBD)

✨ Feature request
Status

Needs work

Version

11.0 πŸ”₯

Component
recipe systemΒ  β†’

Last updated less than a minute ago

Created by

πŸ‡ΊπŸ‡ΈUnited States jnicola

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jnicola
  • πŸ‡ΊπŸ‡ΈUnited States 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.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Failed
    about 2 months ago
    Total: 177s
    #169148
  • Pipeline finished with Failed
    about 2 months ago
    Total: 176s
    #169153
  • Pipeline finished with Failed
    about 2 months ago
    Total: 172s
    #169169
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 51s
    #169179
  • Pipeline finished with Success
    about 2 months ago
    Total: 558s
    #169180
  • Issue was unassigned.
  • Status changed to Needs review about 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States jnicola

    Ready for review by others.

  • πŸ‡ΊπŸ‡Έ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, 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.

  • Status changed to Needs work about 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • πŸ‡ΊπŸ‡Έ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.

  • Pipeline finished with Canceled
    about 2 months ago
    #169881
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 65s
    #169888
  • Pipeline finished with Failed
    about 2 months ago
    Total: 175s
    #169891
  • Pipeline finished with Failed
    about 2 months ago
    Total: 328s
    #169906
  • Pipeline finished with Failed
    about 2 months ago
    Total: 176s
    #169912
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 205s
    #169913
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 4s
    #169918
  • Pipeline finished with Canceled
    about 2 months ago
    #169919
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

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

    It will, however, need explicit test coverage.

  • Pipeline finished with Canceled
    about 2 months ago
    #169920
  • Pipeline finished with Success
    about 2 months ago
    Total: 574s
    #169928
  • Pipeline finished with Success
    about 2 months ago
    Total: 615s
    #169940
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 432s
    #169947
  • Pipeline finished with Failed
    about 2 months ago
    Total: 188s
    #169950
  • Pipeline finished with Success
    about 2 months ago
    Total: 664s
    #169955
  • Pipeline finished with Failed
    about 2 months ago
    Total: 180s
    #169975
  • πŸ‡¬πŸ‡§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 jnicola

    Moving to the new recipe component!

  • πŸ‡ΊπŸ‡ΈUnited States jnicola
  • πŸ‡ΊπŸ‡ΈUnited States jnicola
  • Pipeline finished with Failed
    about 1 month ago
    Total: 168s
    #175413
  • πŸ‡ΊπŸ‡Έ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!

Production build 0.69.0 2024