Skip to content

[Finder] Fix initial directory is opened twice #50884

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

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jul 5, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #50851
License MIT
Doc PR no

The issue was introduced in #8120 which was coded wrongly for several reasons:

  • seekable support is about file/stream content seeking, not about rewind support, non-seekable stream handler can support rewind - it worked by coincidence
  • the original code involved additional directory open (syscall), which is/was slow
  • 1st rewind of DirectoryIterator is implicit, ie. it is not needed - again, skipping the rewind worked by coincidence
  • 2nd+ rewind must fail if it is not supported, the original code silently skipped it

The test from #8120 is kept and is passing.

I also improved the ftp support detection to make the test stricter.

To support rewind for ftp I opened - php/php-src#11597 - but this Symfony PR does not need it.

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@nicolas-grekas
Copy link
Member

Thank you @mvorisek.

@nicolas-grekas nicolas-grekas merged commit 99fa865 into symfony:5.4 Jul 6, 2023
@mvorisek mvorisek deleted the fix_50851 branch July 6, 2023 06:47
@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 6, 2023

@nicolas-grekas the merge from 5.4 to 6.2 branch kept an unused property, please apply the following patch to 6.2 branch and merge up:

diff --git a/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php b/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php
index 8bb9b70..34cced6 100644
--- a/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php
+++ b/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php
@@ -25,7 +25,6 @@ class RecursiveDirectoryIterator extends \RecursiveDirectoryIterator
 {
     private bool $ignoreUnreadableDirs;
     private bool $ignoreFirstRewind = true;
-    private ?bool $rewindable = null;
 
     // these 3 properties take part of the performance optimization to avoid redoing the same work in all iterations
     private string $rootPath;

@nicolas-grekas
Copy link
Member

Done in 8a28b59, thanks for checking.

This was referenced Jul 29, 2023
ben-challis pushed a commit to ben-challis/sql-migrations that referenced this pull request Jul 30, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [amphp/file](https://togithub.com/amphp/file) | require | patch |
`3.0.0-beta.6` -> `3.0.1` |
| [symfony/finder](https://symfony.com)
([source](https://togithub.com/symfony/finder)) | require | patch |
`6.3.0` -> `6.3.2` |

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>amphp/file (amphp/file)</summary>

### [`v3.0.1`](https://togithub.com/amphp/file/releases/tag/v3.0.1):
3.0.1

[Compare
Source](https://togithub.com/amphp/file/compare/v3.0.0...v3.0.1)

#### What's Changed

- Fix `touch()` on non-existent files in ext-uv and ext-eio by
[@&#8203;kelunik](https://togithub.com/kelunik)
([#&#8203;73](https://togithub.com/amphp/file/issues/73))
- Fix `write()` truncation with ext-uv and ext-eio by
[@&#8203;danog](https://togithub.com/danog) in
([#&#8203;76](https://togithub.com/amphp/file/issues/76))

**Full Changelog**:
amphp/file@v3.0.0...v3.0.1

### [`v3.0.0`](https://togithub.com/amphp/file/releases/tag/v3.0.0):
3.0.0

[Compare
Source](https://togithub.com/amphp/file/compare/v3.0.0-beta.6...v3.0.0)

Stable release compatible with AMPHP v3 and fibers! 🎉

As with other libraries compatible with AMPHP v3, most cases of
parameters or returns of `Promise<ResolutionType>` have been replaced
with `ResolutionType`.

-   Renamed `BlockingDriver` to `BlockingFilesystemDriver`
-   Renamed `EioDriver` to `EioFilesystemDriver`
-   Renamed `ParallelDriver` to `ParallelFilesystemDriver`
-   Renamed `StatusCachingDriver` to `StatusCachingFilesystemDriver`
-   Renamed `UvDriver` to `UvFilesystemDriver`
-   Renamed `Amp\File\Sync\AsyncFileMutex` to `Amp\File\FileMutex`
-   Added `?Cancellation` as first parameter of `File::read()`
-   Added `File::isSeekable()`
-   Removed `File::SEEK_SET`, `File::SEEK_CUR`, and `File::SEEK_END`
-   Added `Amp\File\Whence` for seeking instead

</details>

<details>
<summary>symfony/finder (symfony/finder)</summary>

### [`v6.3.2`](https://togithub.com/symfony/finder/releases/tag/v6.3.2)

[Compare
Source](https://togithub.com/symfony/finder/compare/v6.3.0...v6.3.2)

**Changelog**
(symfony/finder@v6.3.1...v6.3.2)

- bug
[symfony/symfony#50884](https://togithub.com/symfony/symfony/issues/50884)
\[Finder] Fix initial directory is opened twice
([@&#8203;mvorisek](https://togithub.com/mvorisek))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/ben-challis/sql-migrations).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4yNC4yIiwidXBkYXRlZEluVmVyIjoiMzYuMjQuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants