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

Lock copied files to prevent removing files needed by other recipes #451

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@dbrumann
Copy link

dbrumann commented Dec 13, 2018

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.

@dbrumann dbrumann force-pushed the dbrumann:feature/lock_files branch from 83a6f1f to 833ff9c Dec 13, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 20, 2018

(rebase needed)

@dbrumann dbrumann force-pushed the dbrumann:feature/lock_files branch 3 times, most recently from fe8e7e3 to d9d8b04 Dec 23, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 22, 2019

Hi @dbrumann, friendly ping to know the status here :)

@dbrumann

This comment has been minimized.

Copy link
Author

dbrumann commented Jan 23, 2019

Hi @nicolas-grekas. Sorry for not updating. I'm currently busy settling into a new client project.

Writing tests for this was a bit tricky due to the Lock-class requiring a file to write to. I think the rebase and the flexible root-dir made this a bit easier, but I had no time to confirm this. I don't feel comfortable mocking the calls to Lock as I want to ensure the file list is handled properly.

I won't have time until Wednesday next week though. I set the end of the first week of February as my personal deadline for this.

@dbrumann dbrumann force-pushed the dbrumann:feature/lock_files branch from d9d8b04 to 05efd92 Feb 8, 2019

@dbrumann dbrumann changed the title [WIP] Lock copied files to prevent removing files needed by other recipes Lock copied files to prevent removing files needed by other recipes Feb 8, 2019

@dbrumann

This comment has been minimized.

Copy link
Author

dbrumann commented Feb 8, 2019

Hey @nicolas-grekas I think this is ready. Let me know if something is missing for you.

@dbrumann dbrumann force-pushed the dbrumann:feature/lock_files branch 3 times, most recently from 252e51d to ae10d8c Feb 9, 2019

@dbrumann
Copy link
Author

dbrumann left a comment

There seems to be an issue after my last changes. When manually trying to verify the fix right now, it did not seem to work. I will update as soon as I figure out what went wrong.

The issue is fixed. I messed up when replacing my array_map-construct with array_column.

@dbrumann dbrumann force-pushed the dbrumann:feature/lock_files branch from 758b2dd to f8cf1a2 Feb 9, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

Tested locally, works like a charm!

@dbrumann dbrumann force-pushed the dbrumann:feature/lock_files branch from f8cf1a2 to 62f8662 Feb 9, 2019

@nicolas-grekas nicolas-grekas referenced this pull request Feb 16, 2019

Open

New release ? #474

@xabbuh

xabbuh approved these changes Feb 17, 2019

Adds file locking for CopyFromRecipe.
- Allows passing lock to configurators.
- Stores copied files in lock.
- Ignores locked files on remove.

@dbrumann dbrumann force-pushed the dbrumann:feature/lock_files branch from 62f8662 to 896e709 Feb 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment