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

Handling of already existing files when requiring/removing a pack #428

Closed
Lyrkan opened this issue Oct 15, 2018 · 1 comment · Fixed by #451
Closed

Handling of already existing files when requiring/removing a pack #428

Lyrkan opened this issue Oct 15, 2018 · 1 comment · Fixed by #451

Comments

@Lyrkan
Copy link

Lyrkan commented Oct 15, 2018

Hi there,

I've seen multiple issues opened on the Encore repository that were caused by an already existing package.json file when requiring the related pack (the most recent one being symfony/webpack-encore#411).

Since the recipe adds this file using the copy-from-recipe configurator it fails silently when the user already has one (which may not be an uncommon situation and means that Encore won't be installed).

Also annoying: if the user decides to remove the pack it will also remove the package.json file (which has probably been modified by then) without any warning.

The first issue could probably be handled by adding a message if copy-from-recipe wasn't able to copy a file for this reason, but:

  • I'm not sure that it will be enough for the users to know what they have to do (even though we could add something in the doc about it I'm afraid it won't be that obvious)
  • There may be some situations where that message would probably not be needed and have the opposite effect (people being worried about a warning message that they could safely ignore)

For the second problem, package.json could probably be ignored in the removeFiles method the same way it's currently done for the ".git" repository, but I'm not sure that adding something that specific to a recipe into Flex's code would be a good idea...

@maxhelias
Copy link
Contributor

👍 for add a message when the file isn't copy.

For the second problem, I do not know if it's possible, but in Flex.php we have the askAndValidate method to ask the user. Is it possible to add it to the configurator and add an option to force the remove without asking?

nicolas-grekas added a commit that referenced this issue Feb 21, 2019
…recipes (dbrumann)

This PR was merged into the 1.2-dev branch.

Discussion
----------

Lock copied files to prevent removing files needed by other recipes

This PR modifies the `CopyFromRecipeConfigurator` so that it will store a list of copied files in symfony.lock and when a recipe is about to be unconfigured, it will check the list of locked files to prevent removal. This should fix #428

## How to reproduce the issue

```
composer create-project symfony/skeleton example
cd example
composer remove console
```

This will remove both files installed through the recipe `bin/console` and `config/bootstrap.php`. As `symfony/framework-bundle` relies on the bootstrap-file as well, when trying to access the website, you will get an error due to the missing file.

## How to verify the fix

Create a new project based on `symfony/skeleton` and replace flex inside the example project with a local copy of this branch:

```
composer create-project symfony/skeleton example
cd example
rm -rf vendor/symfony/flex
ln -s path/to/local/flex vendor/symfony/flex
```

Since the `symfony.lock` was created during setup of the project it will not yet keep track of the copied files and needs to be updated first. An easy way to do this, is to remove the current file and run `composer fix-recipes` to create a new one:

```
rm symfony.lock
composer fix-recipes
```

Alternatively removing a recipe and then requiring it again will update the lock as well, but it needs to be done for each recipe, as otherwise the list of locked files would be incomplete.

Now when we remove the console again, opening the page in the browser should still work and the file `config/bootstrap.php` should still exist as it is locked by the framework-bundle.

## BC Breaks

None, as far as I can tell. For newly required recipes the files will be tracked. The new key for files in `symfony.lock` was not previously used and should not interfere with existing behavior.

## Known limitations

Unfortunately if any of the previously installed recipes has conflicting files, this will not be recognized unless the recipe is removed and then installed again.

Commits
-------

896e709 Adds file locking for CopyFromRecipe.
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 a pull request may close this issue.

2 participants