-
-
Notifications
You must be signed in to change notification settings - Fork 587
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
fix: pass all valid options to nested commands in ddev composer create
, fixes #6300, fixes #6246, for #5058
#6303
Conversation
Download the artifacts for this pull request:
See Testing a PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add test coverage for this? Thanks! We have been fiddling with ddev composer create
for years and years now, and we keep incrementally improving it, but we never seem to completely get it finished.
Yes, I will add some new test or modify an existing one. |
a693d1f
to
4f748c8
Compare
I added more conditions and checks to the test, now it covers most possible situations. |
composer-create
, fixes #6300composer-create
, fixes #6300, for #5058
Confirmed. This PR fixes #6300 . Thank you for the quick turn around. Test
$ ddev -v
ddev version v1.23.1-62-g4f748c85a
mkdir my-cakephp-site && cd my-cakephp-site
ddev config --project-type=cakephp --docroot=webroot
ddev composer create --prefer-dist --no-interaction cakephp/app:~5.0
ddev cake
ddev launch Note This PR updates the quickstart to include |
There is still some difference how composer scripts work in:
I will investigate this further today and adjust the behavior. |
4f748c8
to
a5635db
Compare
The order was:
The right order:
This is why I made This is not a breaking change, because I moved If you passed If I refactored the test logic to make it more clear and added more tests. I removed the testing for the |
composer-create
, fixes #6300, for #5058composer-create
, fixes #6300, fixes #6246, for #5058
composer-create
, fixes #6300, fixes #6246, for #5058ddev composer create
, fixes #6300, fixes #6246, for #5058
When I split combined short flags, it broke |
a5635db
to
b4ae1d4
Compare
Rebased. I fixed I updated our documentation with a more detailed explanation of what steps we do with I researched the Composer documentation, learned more about what
I'm pretty sure the most of the original logic here works correctly. There are some conflicts with the |
b4ae1d4
to
50f7a91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked through the docs, not code, had minor comments.
@rpkoller would you be willing to validate the quickstarts with this PR? I know it's loads of work, but your attention is so valuable. |
a7377e4
to
119ea4b
Compare
argh that slipped through again. was on my to do list then i completely forgot. will try to run through either tomorrow or on thursday hopefully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, and THANK YOU, but we're in hell here and we're going to have to figure out how to get out of it. Either ddev composer create
was a bad idea to begin with (probably - it was my idea) or we've just implemented it badly. But we've been struggling with it for years, and this code is now completely incomprehensible and unmaintainable.
The original idea was just to allow composer create-project
without doing it in a subdirectory. I'm not sure why we didn't end up doing all the work in a temporary directory and then copy it into the project, and I'm not sure why we can't do that now.
Bottom line: This is great and amazing work, but we have to find our way out of this problem.
Note that a --no-interaction
install of Craft CMS does not result in a working site, because it still needs to do a ddev craft setup
to work. (#6246)
--no-interaction
does work fine with CakePHP.
IMO this can go in if @rpkoller is OK with the impacts on the quickstarts.
osargs = nodeps.RemoveItemFromSlice(osargs, "--preserve-flags") | ||
var expandedOsargs []string | ||
for _, osarg := range osargs { | ||
// If this is a combined short option like "-test", split it into "-t -e -s -t" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird that we have to do this, when Cobra has these capabilities. I know there's lots of history in this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we are using Cobra here, because osargs = os.Args[3:]
at the top
In any case, there is always a way to improve it in the future.
composerCmd = append(composerCmd, osarg) | ||
} | ||
for _, validCreateArg := range validCreateArgs { | ||
if isValidComposerOption("install", validCreateArg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting this out.
@@ -329,8 +379,6 @@ for basic project creation or 'ddev ssh' into the web container and execute | |||
} | |||
|
|||
func init() { | |||
ComposerCreateCmd.Flags().BoolP("yes", "y", false, "Yes - skip confirmation prompt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always great to get evil things out of the init() functions
It sounds like a challenge and I like it. I haven't touched any code related to the mutagen, a few restarts here seems to be redundant. But still, I think I'll come back to it some time.
I don't think we can update this quickstart with @mandrasch's technique right now, but after some time when this code is mature, we can update it for |
Just as sidenote: I wondered about this when I first tried to understand why there is a special command wrapper for composer create in ddev. But I haven't had time to find out why (seems like there is git involved, so directory really must be empty? composer/composer#7455 (comment)) Complicated stuff! 🤓 Thanks for all the effort which went into making this work! 🙏 |
Regarding CraftCMS, it's a bit confusing - the official docs state this (https://craftcms.com/docs/5.x/install.html#quick-start): mkdir my-craft-project
cd my-craft-project/
ddev config --project-type=craftcms --docroot=web --php-version=8.2
ddev composer create -y --no-scripts "craftcms/craft"
ddev craft install Using |
* docs: Warn about Codespaces (ddev#6321) [skip ci] Co-authored-by: Matthias Andrasch <777278+mandrasch@users.noreply.github.com> * docs: Improve ddev debug test by offering suggestions early [skip ci] (ddev#6323) * feat: add support for MariaDB 11.4 LTS, fixes ddev#6061 (ddev#6243) Co-authored-by: Stanislav Zhuk <stasadev@gmail.com> * docs: pacify textlint on mysql/https (ddev#6336) [skip ci] * docs: revisit WSL Docker Desktop setup instructions (ddev#6330) * fix: autodownload yarn from corepack, and set cache-folder, fixes ddev#6332 (ddev#6333) * docs: windows-wsl2-dd needs apt install for ngrok (ddev#6341) [skip ci] * build: check docker-compose 2.28.1 (ddev#6340) [skip ci] * fix: use real path to global config location, fixes ddev#6328 (ddev#6329) * test: stop running nightly tests [skip ci] (ddev#6347) * test: Improve reliability of TestProcessHooks (intermittent failures), fixes ddev#6313 (ddev#6314) * fix: Drupal 10 and Drupal 11 settings have migrated, set state_cache (ddev#6346) [skip ci] Co-authored-by: Stanislav Zhuk <stasadev@gmail.com> * build: use IPv4 inside a container, mariadb-client 11.4 for the webserver (ddev#6334) [skip ci] * fix: pass all valid options to nested commands in `ddev composer create`, fixes ddev#6300, fixes ddev#6246, for ddev#5058 (ddev#6303) Co-authored-by: Randy Fay <randy@randyfay.com> Co-authored-by: Ralf Koller <1665422+rpkoller@users.noreply.github.com> * docs: fix up textlint complaints * Turn off defaultTerms * Use exclude properly with textlint for VS Code * update for additional textlint complaints * remove redundant empty exclude section * Try to get textlint/reviewdog to report --------- Co-authored-by: Matthias Andrasch <777278+mandrasch@users.noreply.github.com> Co-authored-by: hussainweb <hussainweb@gmail.com> Co-authored-by: Stanislav Zhuk <stasadev@gmail.com> Co-authored-by: Ralf Koller <1665422+rpkoller@users.noreply.github.com>
@rfay and I discussed our current implementation for I did that and it generally worked, but when I started testing it with our quickstarts, things went wrong. Our current logic:
For example: Laravel quickstart creates CraftCMS asks some interactive questions and without DDEV override it suggested me this:
instead of
So we are tied to the current implementation. |
Thanks for studying so carefully! |
Hey, just my two cents on this. :) I think there are two big questions for quickstarts:
If DDEV doesn't want to be automagic, but just a generic tool - there is a bit more work for the user: In case of Craft CMS, you could write the db connection settings to
(You can also pass arguments to the craft install/craft command like When it comes to interactive dialogs - the values from In a normal non-DDEV scenario, users would do something like As far as I understand, DDEV does the magic for So as far as I see it, something like this could also be possible when there would be a new command for setting the ddev config --project-type=craftcms --docroot=web --php-version=8.2
# regular composer command
ddev composer create-project -y --no-scripts "craftcms/craft"
# handle .env with new ddev command --> runs craftCmsPostStartAction()
ddev setup-env-values
# install cms with CMSes regular CLI command, evaluates .env and so prefill should be correct ddev value
# ddev craft install/craft or typo3 install:setup Personally, I'm not a big fan of automagic - I like to understand what's happening, A new command like On the other hand there are tools like WP Local now with a lot of automagic 🤷 There are also two (obvious) downsides for a new command like
Possible advantages:
|
And it also restarts Mutagen for macOS users.
We could probably review what we do inside
For CraftCMS: it runs If we move to the original behavior for
|
Great additions, thanks for insights @stasadev 👍👍 After reading your post, I noticed the (third) big question is: Does DDEV want devs to follow DDEV quickstart or the official docs? Because https://laravel.com/docs/11.x/installation for instance also says "run migrate if you need db". My personal experience is that DDEV automagic also might hide some important commands which are needed for deployment later on (but this argument is only true for inexperienced beginners + I wasn't aware of all the post create hooks in Composer since there were not executed in the past in DDEV. Otherwise setting up .env correctly is major dev skill i would say... 🤔 But your arguments are of course stronger) |
PS: I also wonder what framework/CMS creators would prefer - no scripts or with scripts? 🤔 PPS: When I think about a DDEV UI in a far future (maybe some day 😁), this would also be a question - guess no-interaction would be needed here. (And thanks for the exchange, super interesting to learn more about how this works👍Maintainability for you and the team/community is of course key) |
That's a good question. Usually, the
Yes, it's in the Laravel documentation and also in the |
We want devs to be successful in their own world, and be comfortable there. The quickstarts are normally adaptations of the official docs. But where the official docs don't reflect a docker-based environment (or reflect an opinionated one of their own) we have to adapt (and usually simplify). Sometimes we do too much for the user. Drupal users can get so complacent about what DDEV does with settings that they forget how the whole settings system works and then they don't know how to debug it, which a few years ago would have been natural and easy for every user. |
The Issue
ddev composer create
fully compatible withcomposer create-project
#5058Composer options were not respected by
composer run-script
.How This PR Solves The Issue
Makes
ddev composer create
fully compatible with all possible composer options. I removed all the hard-coded options.Adds composer options to
composer run-script
.Adds
--no-interaction
to CakePHP quickstart.--preserve-flags
(which doesn't add--no-plugins
and--no-scripts
) didn't work as expected, I decided to remove it.This is not a breaking change, because
--preserve-flags
didn't work before (it was alwaysfalse
).I moved
composer run-script post-create-project-cmd
outside the condition forcomposer install
, to make it work the same way as incomposer create-project
.If you passed
--no-scripts
, DDEV tried to runcomposer run-script
, which was wrong behavior, fixed it.If
--help
or--version
flags were provided, composer showed it's help page or it's version, and DDEV tried to continue creating the project, which was wrong behavior, fixed it.I refactored the test logic to make it more clear and added more tests. I removed the testing for the
psr/log
package and replaced it with custom-made fake packages.Manual Testing Instructions
ddev composer create
must be the same as forcomposer create-project
.To check this behavior, you can use a test repository (it is used for
testdata
here) which contains all the necessary situations to check:test/ddev-composer-create
package (.ddev/test-ddev-composer-create/composer.json
) contains this:test/ddev-require
fromrequire
section:vendor/test/ddev-require
will be here, but only if there is no--no-install
flagtest/ddev-require-dev
fromrequire-dev
section:vendor/test/ddev-require-dev
will be here, but only if there is no--no-dev
/--no-install
flagpost-root-package-install
script:composer install
created-by-post-root-package-install
file, but only if there is no--no-scripts
flagpost-create-project-cmd
script:composer install
created-by-post-create-project-cmd
file, but only if there is no--no-scripts
flagAnd use different flag combinations to see how it works for
composer create-project
andddev composer create
. It is important to check if the commands run in the same order.Available flags for
composer create-project
https://getcomposer.org/doc/03-cli.md#create-projectIgnore the
test-ddev-require
andtest-ddev-require-dev
directories created in the project root (they are expected to be created every time as they are part of the composer package).Every
ddev composer create
will have this output:You can see what flags are used for each of these composer commands, and flags like
--no-interaction
will be also used forcomposer run-script
andcomposer install
.For example:
This should not create
vendor
folder, and there should be nocreated-by-*
filesThis should create
vendor/test/ddev-require
folder, (but notvendor/test/ddev-require-dev
) and there should be nocreated-by-*
filesAnd so on.
Steps To Reproduce
example (it should not ask for input) from:Automated Testing Overview
Release/Deployment Notes