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
[Filesystem] Add targetIterator for fs mirror #40689
Conversation
Thank you. Could you add a test for this please? Also make sure Fabbot is happy. |
Thanks, will do on Monday |
@Nyholm can you help me understand why tests for this pass on my local ( |
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 |
Thanks! This is ready for review. I don't believe the remaining failure is related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I have two questions that will help me understand the PR and the issue better.
Okay, I've rewritten this in a way that's backwards-compatible. |
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? |
Co-authored-by: Alexander M. Turek <me@derrabus.de>
Co-authored-by: Alexander M. Turek <me@derrabus.de>
e451222
to
827ec62
Compare
I rebased on 6.1 and tests are passing again. |
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. |
Co-authored-by: Alexander M. Turek <me@derrabus.de>
Co-authored-by: Alexander M. Turek <me@derrabus.de>
Thanks, I've addressed all feedback. Just to confirm, this will be backported to 5.4 after being merged into 6.1? |
No. New features are never backported. |
I'd argue this is a bug, not a new feature. The expected behavior if 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. |
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 |
@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. |
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.