Closed (fixed)
Project:
Stage File Proxy
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
29 Sep 2017 at 14:17 UTC
Updated:
29 May 2020 at 20:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
JulienD commentedComment #3
JulienD commentedServices are injected using dependency injection
Comment #4
sylvainm commentedThe command should be added in a drush.services.yml, think I
Comment #5
sylvainm commentedAnd in composer.json.
Patch attached
Comment #6
sylvainm commentedComment #7
sylvainm commentedsorry, #5 was wrong…
Comment #8
geek-merlinThanks for working on that! As a maintainer, i see the issue that this patch will introduce code duplicated in 2 places which makes any subsequent patch cumbersome and error-prone. Fortunately the config-split guys have solved that problem well and - even better - documented their effort. It's super simple.
So if this patch is reworked that way, we can move it in.
(Please also include #2921750: Drush command doesn't work with verify option)
Comment #9
geek-merlinAlso see RTBC #2921750: Drush command doesn't work with verify option.
Comment #10
fgmNew version, tested with Drush 9.2.
Comment #11
geek-merlinWhat did you change? As far as i see it, stage_file_proxy.drush8.inc still contains dup code to the command service. See #8.
Comment #12
fgmI did not "change" anything: I hadn't seen the earlier implementation so it has been redone independently. The situation wrt drush 8/9 code is similar to the one about Features, it seems.
Comment #13
jeroent* Removed code duplication
* Replaced deprecated Unicode::* calls with mb_*.
* Fixed some coding standards
* Added #2921750: Drush command doesn't work with verify option.
Comment #14
jeroentComment #16
jeroentfixed coding standards.
Comment #18
jeroentThis is the right patch.
Comment #20
jeroentComment #21
chi commentedDrush 10 has just been released. It would be good to support it in this patch.
https://github.com/drush-ops/drush/releases/tag/10.0.0
Comment #22
greggles@Chi did you test it? An example of upgrading is #3071228: Drush 10 compat which seems pretty lightweight.
Comment #23
betz commentedi am currently testing it from the contribution day on drupalcon :)
well, just the drush 9 one.
will report that
Comment #24
betz commentedpatch fails against the 8.x-1.x branch.
Comment #25
betz commentedComment #26
chi commented@greggles, right, supporting Drush 10 is likely be one line change. So it makes sense to make it in this issue.
Comment #27
betz commentedmade the patch apply to 8.x-1.x branch again.
did not test yet.
Comment #28
betz commentedconfirms patch @27 works for drush 9
Comment #29
betz commentedcould we get this patch already submitted and have drush 10 for another issue?
It works for 9, this issue is 2 years old...
Comment #30
gregglesI'm fine with the approach of committing this for Drush9 and pushing drush10 to another issue in general, though the changes for 10 are likely pretty small.
Anyone else want to review this for 9. I personally don't use this functionality so I'm not well suited to test it.
Comment #31
moshe weitzman commentedLooks pretty good. I noticed
"stage_file_proxy.drush.services.yml": "^9"Change this line in composer.json to
^9 || ^10to get full Drush 10 support. Nothing more is needed.The convention is to throw an Exception in cases like this, not just log an error.
Comment #32
gregglesNeeds work based on #31.
Thanks for the review, Moshe!
Comment #33
sylvainm commentedHere is a patch with #31 comment applied.
I'll submit just after a patch adding a skippable progress bar
I tested the two patches and both work
Comment #34
sylvainm commentedAnd here is the patch with the skippable progress bar
Comment #35
matio89 commentedFixed the patch to be applied correctly.
Attached the patch.
Comment #36
moshe weitzman commentedLast patch failed to apply correctly according to CI.
Comment #37
greggles@moshe does #34 look good to you? Seems like it fixes your feedback and addresses another issue and it applies, according to testbot.
Comment #38
moshe weitzman commentedYeah, looks good. There are still a couple instances of $logger->error() but it looks to me like thats not a true command error, but rather reporting results. So $logger->error() is fine for that use.
Comment #40
gregglesGreat, thanks, everyone for your help!
Now committed.