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

feat: make ddev composer create fully compatible with composer create-project #5058

Merged

Conversation

gilbertsoft
Copy link
Member

@gilbertsoft gilbertsoft commented Jul 1, 2023

The Issue

Composer installer plugins and scripts may lead to error while cloning a package with ddev composer create. Some install instructions in the docs therefor use --no-scripts to avoid issues. Users do not know about the special behavior of ddev composer create and the differences to composer create-project therefor adding --no-plugins and --no-scripts by default would avoid issues automatically.

How This PR Solves The Issue

This PR adds the --no-plugins and --no-scripts to the composer create-project command to avoid issues by default and makes the manual usage of these options superfluous.

There may be rare cases where plugins and scripts should be run, therefor the new flag --preserve-flags is introduced to fallback to the old behavior and not add these two options. But normally it should be fine to running scripts and plugins later with composer install.

The composer create command now emulates exactly the behavior of the original create-project command:

  1. clone
  2. ddev only: move to project root
  3. call post-root-package-install script
  4. ddev only: restart project and generate config files if available for project type
  5. call pre-update-cmd or pre-install-cmd script
  6. update or install
  7. call post-update-cmd or post-install-cmd script
  8. call post-create-project-cmd script

Manual Testing Instructions

Craft CMS and Laravel were using the --no-scripts option in the quick start guide and must be tested to still work properly.

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@github-actions
Copy link

github-actions bot commented Jul 1, 2023

@gilbertsoft gilbertsoft marked this pull request as ready for review July 2, 2023 11:35
@gilbertsoft gilbertsoft requested review from a team as code owners July 2, 2023 11:35
@rfay
Copy link
Member

rfay commented Jul 3, 2023

I appreciate the creativity here and look forward to looking at this more, but you should know that ddev composer create has a long history of incremental changes, and there are lots of ways it can go bad in various contexts (including NFS). So I really wouldn't be willing to bring this in before the upcoming release.

@gilbertsoft gilbertsoft force-pushed the refactor/make-composer-create-less-error-prone branch 2 times, most recently from a58f6ee to d104f29 Compare July 3, 2023 10:23
@gilbertsoft
Copy link
Member Author

I appreciate the creativity here and look forward to looking at this more, but you should know that ddev composer create has a long history of incremental changes, and there are lots of ways it can go bad in various contexts (including NFS). So I really wouldn't be willing to bring this in before the upcoming release.

No hurry to get it in. Has to be tested for all project types. Thanks to @rpkoller who makes a great job in testing :)

But this is a big step for ddev composer create and will save us time in implementing project type specific workarounds!

@gilbertsoft gilbertsoft marked this pull request as draft July 3, 2023 11:58
@gilbertsoft gilbertsoft changed the title refactor: make ddev composer create less error-prone feat: make ddev composer create fully compatible with composer create-project Jul 3, 2023
@rfay
Copy link
Member

rfay commented Jul 3, 2023

Manually tested craftcms without the --no-scripts and it worked fine.

@rfay
Copy link
Member

rfay commented Jul 15, 2023

Rebased.

@rfay rfay added this to the v1.23 milestone Aug 7, 2023
@rfay
Copy link
Member

rfay commented Aug 21, 2023

Please make sure when working on this that ddev composer create errors out if no args are provided.

@rfay rfay removed this from the v1.23 milestone Oct 27, 2023
@rfay rfay self-requested a review February 11, 2024 17:11
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

This looks good, and I would postpone this PR, because of the changes in the docs.
People who are looking at the latest docs and using the stable DDEV won't be able to use the quickstart.

@rfay
Copy link
Member

rfay commented Feb 12, 2024

Good point. The flip side is that this will be out of sync with the quickstarts before it gets pulled, but we can survive that.

I sure hope there's nothing bad about this.

But we also need experience with this in the meantime, so it's a catch-22

@rpkoller
Copy link
Collaborator

rpkoller commented Feb 12, 2024

People who are looking at the latest docs and using the stable DDEV won't be able to use the quickstart.

Out of curiosity, cuz i haven't considered that point as a potential problem yet, are there many people using the current stable version of DDEV with the latest version of the docs on ddev.readthedocs.io? I wouldn't have expected that this is the case cuz, at least me myself tend to use the docs that come along or are on par with the version i currently use?

@rfay
Copy link
Member

rfay commented Feb 12, 2024

@rpkoller so often people use the latest version and get confused. I think we should figure out a better way to do this, and maybe have another branch for breaking-docs-changes.

Often we want to update the docs for current users. Sometimes (like here) we don't want to until they have it in their hands.

Most people don't know there are two versions, or know what the two versions mean.

Would love to have you think about this (in a new issue if we don't have one). We've talked about it many times.

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Feb 12, 2024

so often people use the latest version and get confused. I think we should figure out a better way to do this, and maybe have another branch for breaking-docs-changes.

Probably worth opening a discussion thread about this.

@rfay
Copy link
Member

rfay commented Feb 13, 2024

@GuySartorelli https://github.com/orgs/ddev/discussions/5832

@rfay rfay force-pushed the refactor/make-composer-create-less-error-prone branch from 20d2eb9 to 293f7e6 Compare February 21, 2024 23:23
@rfay rfay force-pushed the refactor/make-composer-create-less-error-prone branch from 293f7e6 to 3bace84 Compare February 25, 2024 20:41
@rfay
Copy link
Member

rfay commented Feb 25, 2024

Rebased

@rfay rfay merged commit 8a6addb into ddev:master Feb 26, 2024
17 of 19 checks passed
@rfay rfay deleted the refactor/make-composer-create-less-error-prone branch February 26, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants