feat: avoid having to remove files in ddev composer create #5493

[hanoii]

Basically what I am doing instead of removing everything is:

• If composer root is different from the app root, it validates that it's empty
• If composer root is the same as the app root, it validates that:
  • there's nothing in the directory other than .ddev, .git, .tarballs and, if configured, the docroot.
  • It also adds a method so that each project type can whitelist extra paths (settings, upload dirs, etc..) that would be accepted as valid paths to be present for running composer create

If any of the validations above fails, the command does not run. This forces you to start on a clean directory without doing any removal, if you have to remove things, you have to do it yourself.

Helps with #5493

[hanoii]

GitHub is all over the place - https://www.githubstatus.com/

[hanoii]

rebased to restart tests

[rfay]

I'd be a bit interested in you revisiting the very promising

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

before going forward with this one. If you wouldn't mind taking a look at that and thinking about it, I'm sure we can figure out a way for you to either push into it or just start a new PR by forking it.

[github-actions]

Download the artifacts for this pull request:

• all-ddev-executables.zip
• ddev-linux-amd64.zip
• ddev-linux-arm64.zip
• ddev-macos-amd64.zip
• ddev-macos-arm64.zip
• ddev-windows-amd64-installer.zip

See Testing a PR

[hanoii]

@rfay I looked at #5058 , from what I gathered, the other one is much bigger and not so related to this one. This one just changes the approach to not remove anything and performs useful validations to whether it should run of not. If the two should be combined into one I'd say to fork the otherone on top of this one, but maybe this one is quicker to get accepted? If you like the approach the changes should be pretty harmless.

[rfay]

Thanks! This is going to need tests as well, because it's super important.

And another thing to think about (another PR) is controlling ddev config as discussed in #5493

[hanoii]

@rfay

Thanks! This is going to need tests as well, because it's super important.

I would assume that all of the tests already there for this functionality should be as useful, if you can thing of anything else to test.

One test could be make sure it fails when it has to.

I will comment on #5493 as I don't see that much of a need there if this gets in.

[hanoii]

@rfay can you let me know why on

ddev/cmd/ddev/cmd/composer_test.go

Lines 97 to 114 in 64c9e46

	args = []string{"composer", "create", "--prefer-dist", "--no-dev", "--no-install", "psr/log:1.1.0"}
	out, err = exec.RunHostCommand(DdevBin, args...)
	assert.NoError(err, "failed to run %v: err=%v, output=\n=====\n%s\n=====\n", args, err, out)
	assert.Contains(out, "Created project in ")
	assert.FileExists(filepath.Join(tmpDir, composerRoot, "Psr/Log/LogLevel.php"))
	// Test a composer require, with passthrough args
	args = []string{"composer", "require", "sebastian/version", "--no-plugins", "--ansi"}
	out, err = exec.RunHostCommand(DdevBin, args...)
	assert.NoError(err, "failed to run %v: err=%v, output=\n=====\n%s\n=====\n", args, err, out)
	assert.Contains(out, "Generating autoload files")
	assert.FileExists(filepath.Join(tmpDir, composerRoot, "vendor/sebastian/version/composer.json"))
	// Test a composer remove
	args = []string{"composer", "remove", "sebastian/version"}
	out, err = exec.RunHostCommand(DdevBin, args...)
	assert.NoError(err, "failed to run %v: err=%v, output=\n=====\n%s\n=====\n", args, err, out)
	assert.Contains(out, "Generating autoload files")
	assert.False(fileutil.FileExists(filepath.Join(tmpDir, composerRoot, "vendor/sebastian")))

you run two composer create on the same app. Up to now that would work because the next one would wipe everything out which obivously makes the test fail with this new approach. I am thinking how two modfiy, one option is to remove the second run, or if needed, run them from within new "ddev instances".. just trying to understand exactly what use case you are testing there.

[rfay]

It's just testing different things, but using the same project to do it, which saves time. You could change it to deliberately remove (have the test remove the contents) or you could change it to create new projects.

[hanoii]

@rfay this obviously adds a step to some quickstarts, see https://ddev--5499.org.readthedocs.build/en/5499/users/quickstart/#drupal for example. As not every project adds files to the webroot on start this should only be on those that do. I can over other composer create quickstart and check that out.

[rfay]

this obviously adds a step to some quickstarts

Please say why :) It's not so obvious.

[rfay]

It's not possible to remove some things if they're bind-mounted, like sites/default/files with mutagen. Not sure where you're going with this.

[hanoii]

It's not possible to remove some things if they're bind-mounted, like sites/default/files with mutagen. Not sure where you're going with this.

I've thought about this, the way I undrestood your current code, you are removing them and then recreating it before the first restart.

I figured this is not necessary, as another restart happens after composer create finishes. As right now ddev is not removing anything, the user is, I thought this was OK. Correct me if I am wrong, and I'd love for you to test this (which is ready to test except from tests).

[hanoii]

see if https://ddev--5499.org.readthedocs.build/en/5499/users/quickstart/#drupal is clear enough now.

[hanoii]

Because you are removing things on the host, i don't think that affects the container, the bind mount will be there on restart.

[rfay]

Not very happy about that :(

[hanoii]

Not very happy about that :(

About what? And can you elaborate what would you recommend doing instead?

[rfay]

rm -fr ../my-drupal10-site/web/* is

1. Not going to work in many cases
2. Open to failures by people not familiar with the command line
3. Using * with rm should never be done by people casually

Why is the code not doing this programatically?

[hanoii]

Why is the code not doing this programatically?

Well, the main goal of this PR was to offload arbitrary removal of ddev to the user, so that ddev doesn't have any chance of removing unnecessary things. As it is down, doing a composer create on a wrong folder could have a very bad data loss, that's why I consider this safer.

But safer doesn't mean user friendly and you know more about your users than me. I used * on purpose as that means anything inside that dir, which is less prone to issues.

But I thought of something else that was more work so I didn't go that route, which is removing ONLY what ddev start adds, which probably means adding another hook to your projectMatrix... I can explore that. I prefer not go the route of having ddev remove EVERYTHING inside the docroot, as again, there might be things there that ddev should not remove.

Again, let's say you accidentally init in a parent dir that happen to have a web docroot and you wiped that out, not great.

[rfay]

I guess we're finding out why composer create-project refuses to operate on anything but a completely empty directory.

Another strategy may be to gleefully remove composer_root/vendor, but balk at other subdirectories.

[rfay]

For the quickstarts, since people are clearly in a new directory, we should be able to sort this out.

[rfay]

Some other possible safety valves:

• refuse to operate if .ddev directories were found in subdirectories; perhaps there are other red flags we could use.
• Don't try to remove dotfiles

[rfay]

I got to spend some time with this and it's good to go, thanks!

I added one commit, please take a look and see if you're OK with it @hanoii .

It's great that this prevents accidental problems using this command, and I think even if there are any flaws people will understand and clean up their project successfully.

This can go into v1.22.5 this week.

[hanoii]

@rfay all good with your last commit!

[gitressa]

Thanks @hanoii and @rfay! Maybe add this in the issue summary, so this PR and its issue gets linked and automatically closed as well? Closes #5493. I tried to see if I could link from an issue to a PR, but it doesn't look like it ...

A very impressive effort, and getting the feature available already in the next release is great!

[hanoii]

@gitressa thanks! it's already merged so adding that wouldn't make a difference, and also, I think that @rfay steer that isssue into another direction which goes beyond what this PR fixes, which is running ddev config in wrong directories.

[gitressa]

All right, well as long as a lot of folders are not deleted accidentally I am glad, so I closed the issue, and will update the title :)

EDIT: Thanks for clarifying the situation in #5493, I simply misunderstood that the original issue and this PR diverged.