Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
During the Acquia Large Scale Drupal hackathon in London we decided to port this module to Drupal 8.
Here is the patch for review based on the 7.x-1.x branch :
- Moved the module's custom variables to a settings yml file to allow the new configuration management system to handle them as part of the configuration.
- Replaced the deprecated hook_init() implementation to a EventSubcriber named ProxySubscriber.
- Created a FetchManager service with its own interface to fetch the files.
Maybe some code should be moved from the ProxySubscriber to the FetchManager for better separation.
The current logic match the previous version very well but maybe we can improve that logic to give more flexibility to the module.
We still need to write tests but the basic functionalities work.
Comment | File | Size | Author |
---|---|---|---|
#28 | 2206585-28.patch | 17.49 KB | webflo |
Comments
Comment #1
nicoloye CreditAttribution: nicoloye commentedComment #2
gregglesVery cool - thanks!
Should I commit this now so that there's a base for people to use for future work?
Comment #3
dixon_@greggles That would be lovely! :)
Comment #4
nicoloye CreditAttribution: nicoloye commentedWith pleasure, thanks !
Comment #5
gregglesAwesome, thanks!
Committed at http://drupalcode.org/project/stage_file_proxy.git/commit/0250955 with credit to nicoloye and the event (not sure if you had help from others, but seems worth mentioning).
I created a release https://drupal.org/node/2206807 that will be published in a few hours and will then try to remember to add it the project page and update this issue to be "version 8.x-1.x-dev"
Comment #6
nicoloye CreditAttribution: nicoloye commentedGreat !
Thanks for the event credit, it was indeed worth mentioning.
We've worked together with dixon on this port, great experience.
Comment #7
webflo CreditAttribution: webflo commentedUpdates some docs and the use statements for Guzzle.
Comment #8
webflo CreditAttribution: webflo commentedUpdate to prs4, wrote config schema, removed a few deprecated function calls and injected the logger.
The latest version is up on github. https://github.com/ueberbit/stage_file_proxy
Comment #9
webflo CreditAttribution: webflo commentedComment #10
greggles@nicoloye, @dixon_ are either of you able to review the work in #8?
Comment #11
nicoloye CreditAttribution: nicoloye commentedI tested the patch on a drupal 8 beta 9.
The module do enable properly but then I have this :
Fatal error: Call to undefined function Drupal\stage_file_proxy\EventSubscriber\request_path() in /var/www/actency/ddd/0.d8/modules/contrib/stage_file_proxy/src/EventSubscriber/ProxySubscriber.php on line 59
The core has well changed since the first port was done. I'll try to check this in depth if I have some time in the following days.
Comment #12
nicoloye CreditAttribution: nicoloye commentedOK so this is due to https://www.drupal.org/node/2382211.
I tried replacing the old calls by
Url::fromRoute('<current>')
but it doesn't work. The router have already redirected on 404 at this point and all we catch issystem/404
.I'm not sure how I can get the info properly. I'll search.
Comment #13
webflo CreditAttribution: webflo commented@nicoloye i thought tis commit https://github.com/ueberbit/stage_file_proxy/commit/888c91bf68f91af4a30c... would fix the issue but it looks like its not sufficient. Maybe the weights have changed?
Would you please send me a pull request with your changes?
Comment #14
webflo CreditAttribution: webflo commented@nicoloye Please test the linked repo. I should work in beta9. The patch in #8 is outdated.
Comment #15
nicoloye CreditAttribution: nicoloye commentedWe have fixed the issue with a colleague.
There is still a little problem when you are not working in hotlink mode, we will post our patch as soon as it is OK.
Comment #16
nicoloye CreditAttribution: nicoloye commentedAh, we also added the configuration form to the upcoming patch :)
Comment #17
nicoloye CreditAttribution: nicoloye at Actency commentedHere is a new version of the patch done with @Nixou.
This patch complete your work @webflo. We have fixed the redirection problems with or without hotlink.
I have somes troubles with image styles locally so can't validate it works well with that but I think we have most of the work done here. :)
This patch also add the configuration form.
Comment #18
nicoloye CreditAttribution: nicoloye at Actency commentedSorry, small problem of CRLF in the patch file. Fixed here.
Comment #19
webflo CreditAttribution: webflo commented@nicoloye Could you provide a interdiff or pull request against my github repo? Thanks!
Comment #20
nicoloye CreditAttribution: nicoloye at Actency commented@webflo, no problem.
We saw some other small problems here and there, we prepare a rerolled patch and an interdiff for you :)
Comment #21
RenrhafHere is the new patch & interdiff.
Comment #22
RenrhafSome errors are present in the patch, I'll upload a fixed one soon.
Comment #23
RenrhafComment #24
RenrhafThere was still a .info remaining that had to be deleted, sorry for patch flooding...
I added some documentation and injected the stack request.
Comment #25
Dane Powell CreditAttribution: Dane Powell commentedFrom what I can tell, there's already been one commit associated with this issue, and subsequently at least two paths of development (the patch in #17, which includes documentation updates, and the patch in #24, which does not.) Needless to say, this makes it really hard for a newcomer to determine the status of the codebase, much less try to contribute.
Given that we now have a 8.x-1.x release of the module, and people are already opening separate issues against it (#2559527: Exception: 'The service "stage_file_proxy.fetch_manager" has a dependency on a non-existent service "http_default_client".'), let's please close this issue to avoid further confusion.
Comment #26
nicoloye CreditAttribution: nicoloye at Actency commentedHello Dane,
I'll review both patches to see what has to be kept but I think the last one was the good one, now to close the issue we need a maintainer to help reviewing the code and push it to the 8.x-1.0 branch.
Comment #27
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedRerolled the patches from #17 and #24 in one.
Committed everything to https://github.com/ueberbit/stage_file_proxy
Comment #28
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedSame patch as in #27 but with
git diff -M
.Comment #29
nicoloye CreditAttribution: nicoloye at Actency commentedAh nice, thanks webflo :)
Comment #30
Dane Powell CreditAttribution: Dane Powell commentedThanks, I've reviewed #28 and confirmed that it rolls up everything from #17-24 and matches ueberbit's downstream repo. Let's get that committed so we can continue development in dedicated issues.
Comment #32
gregglesOK then.
Thanks everyone for your work on this issue! As suggested by Dane Powell in #25, closing the issue so we can let other fixes happen in other issues.
Comment #33
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThanks Greg!
Comment #34
webflo CreditAttribution: webflo at UEBERBIT GmbH commented@greggles You forgot to add the new
src
folder to your git commit.Comment #35
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #39
gregglesThanks to webflo here and in irc for the assistance in making sure I got everything committed properly.
TIL: "git apply --index" is helpful.
Comment #41
apaderno