-
Notifications
You must be signed in to change notification settings - Fork 0
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
build: refactor URL expansion into a controller trait #351
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #351 +/- ##
=========================================
Coverage 77.07% 77.07%
Complexity 124 124
=========================================
Files 24 24
Lines 301 301
=========================================
Hits 232 232
Misses 69 69 Continue to review full report at Codecov.
|
This should stop codecov from blocking this
*/ | ||
trait PublicUrl | ||
{ | ||
private static function _publicUrl($file, $disk = self::DISK) |
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.
The whole code works, my only concern here is that nothing enforces the class using this trait to have a DISK
property. Do you have any better idea how to enforce the consuming class to have the property this needs? If not feel free to merge this PR as is.
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.
Well, defining a property in the trait would be the best, if we could override it - sadly, it is not simply possible...
https://stackoverflow.com/questions/32571920/overriding-doctrine-trait-properties
I think I might be able to move the trait usage into the controller base class, which I subclass in every other controller...
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.
How about that instead?
A default for a default...
Nope, it won't work, as we will still get an error due to constant self::DISK
not existing.
I do not see any nice way to solve this, but I do not think that it is a big issue.
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.
Sure, then feel free to merge this
Closes #235