[6.x] Resolve issues with silent Artisan failures & FileStore cache flushing #33458
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change subtly adjusts how directory deletions are confirmed. The main change being that we're reactively confirming if the directory was deleted. The original code assumes that the deleteDirectory method is always going to return false when it fails. This is an incorrect assumption to make. Because the deleteDirectory method actually will only return false if the input path isn't a directory. And it will silently fail if it has issues with removal of the directory and/or it's children items.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR has been created in response to #33310 and seeks to resolve that issue and an issue previously discussed as #1179. As well as being a follow up to PR #25254 - to more fully catch the issues that PR attempted to address.
The only flaw (for lack of better term) with the previous PR was that the underlying
FileStore::flush()method wasn't reliably returning false as the result response. This is in turn due to howFilesystem::deleteDirectory()works, since this method simply silently fails if there are issues with removal.Since Filesystem is such a fundamental component and changing
deleteDirectorythat much would surely break tests and be a BC issue. So solving the issue in the FileStore class seems like a good option, since we can be 'defensive' about the results ofdeleteDirectory.While a more elegant solution may exists - and could be a long term goal - this addresses the new reported issue (#33310) and provides a more robust method to enable the previous fix (#25254) to work as intended.