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.

CommentFileSizeAuthor
#28 2206585-28.patch17.49 KBwebflo
#27 2206585-27.patch26.51 KBwebflo
#24 interdiff-2206585-8-24.txt11.3 KBrenrhaf
#24 stage-file-proxy-d8-2206585-24.patch10.72 KBrenrhaf
#23 interdiff-2206585-8-23.txt11.54 KBrenrhaf
#23 stage-file-proxy-d8-2206585-23.patch11.03 KBrenrhaf
#21 interdiff-2206585-8-21.txt16 KBrenrhaf
#21 stage-file-proxy-d8-2206585-21.patch15.5 KBrenrhaf
#18 stage-file-proxy-d8-2206585-18.patch26.82 KBnicoloye
#17 stage-file-proxy-d8-2206585-17.patch27.65 KBnicoloye
#8 stage-file-proxy-d8-2206585-8.patch19.21 KBwebflo
#7 stage-file-proxy-d8-2206585.patch5.7 KBwebflo
stage_file_proxy_d8port.patch14 KBnicoloye

Comments

nicoloye’s picture

nicoloye commented
Issue summary: View changes
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Very cool - thanks!

Should I commit this now so that there's a base for people to use for future work?

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented

@greggles That would be lovely! :)

nicoloye’s picture

nicoloye commented

With pleasure, thanks !

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Awesome, 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"

nicoloye’s picture

nicoloye commented

Great !
Thanks for the event credit, it was indeed worth mentioning.
We've worked together with dixon on this port, great experience.

webflo’s picture

webflo commented
StatusFileSize
new stage-file-proxy-d8-2206585.patch5.7 KB

Updates some docs and the use statements for Guzzle.

webflo’s picture

webflo commented
StatusFileSize
new stage-file-proxy-d8-2206585-8.patch19.21 KB

Update 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

webflo’s picture

webflo commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

@nicoloye, @dixon_ are either of you able to review the work in #8?

nicoloye’s picture

nicoloye commented
Status: Needs review » Needs work

I 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.

nicoloye’s picture

nicoloye commented

OK 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 is system/404.
I'm not sure how I can get the info properly. I'll search.

webflo’s picture

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?

webflo’s picture

webflo commented

@nicoloye Please test the linked repo. I should work in beta9. The patch in #8 is outdated.

nicoloye’s picture

nicoloye commented

We 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.

nicoloye’s picture

nicoloye commented

Ah, we also added the configuration form to the upcoming patch :)

nicoloye’s picture

nicoloye commented
Status: Needs work » Needs review
StatusFileSize
new stage-file-proxy-d8-2206585-17.patch27.65 KB

Here 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.

nicoloye’s picture

nicoloye commented
StatusFileSize
new stage-file-proxy-d8-2206585-18.patch26.82 KB

Sorry, small problem of CRLF in the patch file. Fixed here.

webflo’s picture

webflo commented

@nicoloye Could you provide a interdiff or pull request against my github repo? Thanks!

nicoloye’s picture

nicoloye commented

@webflo, no problem.
We saw some other small problems here and there, we prepare a rerolled patch and an interdiff for you :)

renrhaf’s picture

renrhaf
Primary language French
Location 📍 Strasbourg 🐦🦜
commented
StatusFileSize
new stage-file-proxy-d8-2206585-21.patch15.5 KB
new interdiff-2206585-8-21.txt16 KB

Here is the new patch & interdiff.

renrhaf’s picture

renrhaf
Primary language French
Location 📍 Strasbourg 🐦🦜
commented

Some errors are present in the patch, I'll upload a fixed one soon.

renrhaf’s picture

renrhaf
Primary language French
Location 📍 Strasbourg 🐦🦜
commented
StatusFileSize
new stage-file-proxy-d8-2206585-23.patch11.03 KB
new interdiff-2206585-8-23.txt11.54 KB
renrhaf’s picture

renrhaf
Primary language French
Location 📍 Strasbourg 🐦🦜
commented
StatusFileSize
new stage-file-proxy-d8-2206585-24.patch10.72 KB
new interdiff-2206585-8-24.txt11.3 KB

There was still a .info remaining that had to be deleted, sorry for patch flooding...

I added some documentation and injected the stack request.

danepowell’s picture

danepowell commented

From 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.

nicoloye’s picture

nicoloye commented

Hello 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.

webflo’s picture

webflo commented
StatusFileSize
new 2206585-27.patch26.51 KB

Rerolled the patches from #17 and #24 in one.

Committed everything to https://github.com/ueberbit/stage_file_proxy

webflo’s picture

webflo commented
StatusFileSize
new 2206585-28.patch17.49 KB

Same patch as in #27 but with git diff -M.

nicoloye’s picture

nicoloye commented

Ah nice, thanks webflo :)

danepowell’s picture

danepowell commented
Status: Needs review » Reviewed & tested by the community

Thanks, 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.

  • greggles committed 754c4b8 on 8.x-1.x authored by webflo 
    Issue #2206585 by webflo, Renrhaf, nicoloye: Drupal 8 port
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Reviewed & tested by the community » Fixed

OK 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.

webflo’s picture

webflo commented

Thanks Greg!

webflo’s picture

webflo commented
Priority: Major » Critical
Status: Fixed » Active

@greggles You forgot to add the new src folder to your git commit.

webflo’s picture

webflo commented
Status: Active » Reviewed & tested by the community

  • greggles committed 6f7e388 on authored by webflo 
    Issue #2206585 by webflo, Renrhaf, nicoloye: Drupal 8 port

  • greggles committed f76c94e on 8.x-1.x authored by webflo 
    Issue #2206585 by webflo, Renrhaf, nicoloye: Drupal 8 port

  • greggles committed 6fdb6bc on 8.x-1.x authored by webflo 
    Issue #2206585 by webflo, Renrhaf, nicoloye: Drupal 8 port
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Reviewed & tested by the community » Fixed

Thanks to webflo here and in irc for the assistance in making sure I got everything committed properly.

TIL: "git apply --index" is helpful.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: -#lsdhack