Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Add support for Drupal recipes #36

Open
wants to merge 3 commits into
base: 11.x
Choose a base branch
from

Conversation

szeidler
Copy link
Member

@szeidler szeidler commented Jan 9, 2025

The PR adds support for Drupal recipes. I went for the minimal configuration that is placing recipes into /recipes, not into a subfolder like /recipes/contrib. This is up for discussion and would require the following in Composer .extra

            "recipes/contrib/{$name}": [
                ["type:drupal-recipe"]
            ]

Explanation for the following:

        "drupal-recipe": {
            "unpack-remove-recipe": true,
            "auto-unpack": true
        },

Only works if https://www.drupal.org/project/distributions_recipes/issues/3355485 is resolved or https://github.com/woredeyonas/Drupal-Recipe-Unpack is used.

So if we vote against both the PR is obsolete.

@szeidler szeidler requested a review from jfauske January 9, 2025 11:30
Copy link

@jfauske jfauske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you need a composer normalize.
With this version I get the same error as the bot complains about when doing composer normalize --dry-run.
After composer normalize I get "./composer.json is already normalized."
Unclear to me if it's just some whitespace issue or some "hidden" character.
Edit:
It's of course the order which for some reason I thoiught you had changed...

@szeidler
Copy link
Member Author

szeidler commented Jan 9, 2025

Thanks, there you go :) The main question is probably if we need the installer-paths. I'm wondering if it requires subfolders like /recipes/contrib, because is there even the chance of custom recipes? I can only foresee either contrib recipes or ramsalt ones. Your privacy recipe has the same project type contrib ones.

@jfauske
Copy link

jfauske commented Jan 13, 2025

I think recipes/{$name} will suffice for the large majority of cases. And I'm also wondering if that dir can simply be gitignored... though there could probably be use cases where keeping a recipe around is useful.

I really don't have any opinion here - just think I copied some doc sections from a module and the module/theme structure partly stuck around.

(I see in my recipes, that I mix the variants in the READMEs - Should clean that up to whatever we decide here.)

@szeidler
Copy link
Member Author

Yes, that would be good. I currently cannot see a need of recipes/custom or similar. Of everything comes from external (contrib/ramsalt) we can just use that plain gitignored directory.

Then this PR would be more about adding the gitignore, because the rest of the Composer recipe handling works out of the box.

Copy link

@jfauske jfauske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
And I think I lean in favor of adding this to .gitignore.
It's the result of the recipe run we care about and would commit - keeping the recipe itself around serves no real purpose does it?

@szeidler
Copy link
Member Author

No, it should be gitignored in my opinion. It's just that gitignoring /recipes takes away the possibility to have custom recipes, but on the other hand I do not see a use-case for it and even if so, you can just run it once on your local and that would be enough.

@szeidler
Copy link
Member Author

We might want to wait for https://www.drupal.org/project/drupal/issues/3355485. Core committers want to prioritize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants