Clean up usages / deprecate path helpers #2155
Merged
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
askvortsov1
reviewed
May 9, 2020
askvortsov1
approved these changes
May 12, 2020
luceos
requested changes
May 15, 2020
There was a problem hiding this comment.
I've added some comments. Especially the change to use the system tmp directory isn't really a good idea as the previous implementation tempnam would try the given directory and fallback on the system directory. This implementation now has no failsafe at all if the system tmp directory isn't writable.
4 tasks
luceos
approved these changes
Jun 11, 2020
tankerkiller125
approved these changes
Jun 11, 2020
Less I/O, and one less place where we access the global path helpers.
This was probably taken straight from Laravel. There is no equivalent concept in Flarum, so this should be safe to remove.
Developers using these helpers can inject the `Paths` class instead.
a) The comparison was already outdated, as a different path was passed. b) We're trying to get rid of these global helpers.
franzliedke
force-pushed
the
fl/2055-paths
branch
from
691ac14
to
fd32230
Compare
|
Looks good to merge IMO |
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.
Relates to #2055
Changes proposed in this pull request:
public_path()Applicationclass, e.g.Application::publicPath()All of these are left-overs from our deeper Laravel integration. The recommended way to access these is to use the
Pathsclass, which is injectable as a proper dependency.All remaining changes are cleanups of places where we used the helpers ourselves - some of them quite obscure (e.g. in
WritablePaths).Reviewers should focus on:
Any downsides to removing these? I haven't yet found a case, where refactoring away from the global helpers did not result in an improvement.
Confirmed
Frontend changes: tested on a local Flarum installation.composer test).Required changes: