symfony
symfony copied to clipboard
symfony
[Filesystem] Add targetIterator for fs mirror
| Q | A |
|---|---|
| Branch? | 6.1 |
| Bug fix? | no |
| New feature? yes | |
| Deprecations? | no |
| Tickets | Fix #14068 |
| License | MIT |
The existing $iterator parameter is useless and will not behave as expected if $options['delete'] == true. There needs to be a separate iterator parameter for the target directory.
Thank you. Could you add a test for this please? Also make sure Fabbot is happy.
@Nyholm can you help me understand why tests for this pass on my local (./phpunit src/Symfony/Component/Filesystem/) but fail in CI? It appears that the Finder component is missing in CI.
It appears that the Finder component is missing in CI.
https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Filesystem/composer.json
You can only use packages that appear in the component's dependencies.
@derrabus is correct. You could fix that by adding Finder in the require-dev section.
Or even better, use the glob() to avoid the extra dependency.
Thanks! This is ready for review. I don't believe the remaining failure is related to this PR.
We need to decide if we want to merge this change as a bugfix. @symfony/mergers I need a second opinion for this.
I'm happy to take any approach you decide, just let me know how you'd like to proceed.
Hey, fancy meeting you here, @danepowell! 😄
This is biting me, too, on v5.3.4. @derrabus, can I do anything to move this forward?
We've revised our policy regarding bugfixes recently, and I suppose we need to schedule this for 5.4 as a feature. Can you please do a rebase?
My question from April is still open: If we add a second iterator to the signature, we should have a test scenario in which both iterators are set.
Thanks, I've addressed all feedback.
Just to confirm, this will be backported to 5.4 after being merged into 6.1?
I'd argue this is a bug, not a new feature. The expected behavior if $options['delete'] == true is to delete files in the target directory. But the actual behavior is that files are never deleted, because the origin iterator doesn't apply. That's why we jumped through hoops to make this backwards-compatible.
If we're not going to backport this, the code would be greatly simplified by breaking compatibility and getting rid of that ghost parameter.
@danepowell breaking BC is not allowed in a minor versions. And major versions are only allowed to break BC with a migration path built for it in the previous minor. Backporting it in a patch version of 5.4 or no does not change the need for BC.
Okay, we won't backport it. Any chance of getting this merged? Anything at all I can do to help?
I'm not comfortable with this PR. I think cloning the iterator as suggested would be much better. Maybe replacing the origin directory with the target directory after cloning if part of some iterators (not sure if that's always possible). But asking the user to pass a second iterator does not seem very user-friendly.
Update: Now that I reread the code, the current iterator argument is "broken" as it does not use the originDir anyway. So, I'm :-1: with this PR. I would deprecate the current iterator argument instead.
@fabpot the problem is that iterators contain absolute paths. If the origin and target directories are different, you must have separate iterators for each. Unless you know of some way to hack the iterator to use relative paths? That's beyond my level of comfort, but if you think it's possible and wise I can investigate.
I can no longer reproduce the original issue here, not because it's fixed but just because so much time has elapsed and I'm no longer as familiar with these components. Between that and the obvious resistance to fixing this, I'm going to close it for now.