Add a dedicated file deletion for unconfigure recipes

[maxhelias]

Re-open #667

In this way, we correct the deletion of the same file imported by 2 recipes.
Factoring in this way would allow us to remove the $lock parameter passing in the unconfigure method and with a little more work in the configure.

WDYT about this way ? Can I continue in this direction?

[maxhelias]

Outdated and should be resolve in #845

[nicolas-grekas]

@maxhelias up to rebase this for 2.x?
It looks like we might need this: removing phpunit-bridge currently removes files that should be kept for phpunit when it remains and you had the answer apparently.

[maxhelias]

@nicolas-grekas Yes, of course, I think I'll have to work on it again after all this time 😅

[maxhelias]

@nicolas-grekas Everything looks good to me. To be fair, the solution isn’t entirely mine — it actually comes from a conversation we had about five years ago 😅 #615 (comment)

[nicolas-grekas]

Looks cool thanks!

I don't know if this a real-world concern but the change on the signature of the configurator's constructors is a BC break.
This would break anyone adding their own configurator, if any.
Maybe we can update the code with this concern in mind just to be on the safe side?

It'd mean we could wrap the FilesManager in the Options class possibly? (that'd easy reintroduce the shouldWriteFile method + OptionsTest, which could be marked deprecated / legacy respectively?)

An alternative would be to add a setter on AbstractConfigurator.

WDYT?

[maxhelias]

Yes, it's a BC for custom configurators. These cases should be very limited, though, as the list of configurators is hard-coded and not extensible within the Symfony\Flex\Configurator class

Encapsulating the FilesManager in the Options class seems like the simplest and safest solution. I had initially left out the shouldWriteFile method to better separate concerns -- to keep Options free of any dependency on IOInterface. That said, reintroducing shouldWriteFile through FilesManager feels acceptable in this case, as it avoids any BCs while keeping IOInterface out of Options.

And the alternative, add a setter on AbstractConfigurator. That would preserve separation of concerns, but it would also make configurators mutable. Even though we control the instantiation flow, I'm a bit hesitant about introducing that mutability.

[nicolas-grekas]

I agree, let's go with the first option. 🙏

[nicolas-grekas]

I'm going to take over the PR if you don't, apply my own review comments and a few more things I found while giving it a try. Also because we'd like this to be ready early next week.
Thanks for starting the task!

[maxhelias]

@nicolas-grekas If you want it for the beginning of the week, please take it, I won't have time for that.

I pushed what I'd started at the end of the week as it was.

[nicolas-grekas]

I'm going to open a new PR, the current one is too far from the target, I had to rewrite almost everything :)
Thanks for submitting, this helps a lot to move forward on the topic!

[maxhelias]

Don't hesitate to ping me for a review if needed 😉