- Issue created by @Rajab Natshah
- Status changed to Needs work
5 months ago 8:36pm 20 June 2024 - Status changed to Needs review
5 months ago 10:55pm 20 June 2024 - 🇯🇴Jordan Rajab Natshah Jordan
Attache a static patch file for drimage_improved 2024-06-21 MR 2
To be used with composer patches - 🇯🇴Jordan Rajab Natshah Jordan
Thanks, Alexandre, for reporting 🐛 Fix Drupal 10.3 compatibility for Too few arguments to function deliver() 3 passed in Drimage Needs review
You gave a quick hit on how to fix this issue
Needed to have quick fix - 🇯🇴Jordan Rajab Natshah Jordan
Please, review, still we may need to check if in some cases,
The fix may need to provide a'private'
not to have'public'
all the time. - 🇯🇴Jordan Rajab Natshah Jordan
alxgl commented 21 June 2024 at 11:31
I don't have the time right now to give a better proposition, but I think we should be closer than what have been proposed here for stage_file_proxy, in terms of coding standard : https://git.drupalcode.org/project/stage_file_proxy/-/commit/a26b05f77ad...I had 2nd round of testing reviewing the code in both modules.
The Stag File Proxy module has extended the ImageStyleDownloadController class, with custom override implementation for the
deliver
method.
they had to update that.The Drimage module has extended the ImageStyleDownloadController class but did not override the
deliver
method.
It is only calling it.My only concerns at this time are:
- When to pass
'private'
and when to pass'public'
? - Is using
"public"
all the time is the optimal logic? - Should we add a new override implementation for the
deliver
method to allow for public or private cases?
- When to pass
- 🇯🇴Jordan Qusai Taha Amman
We should use the $scheme variable for more flexibility since, in my case, the scheme will be s3, making it necessary to be dynamic.
- 🇯🇴Jordan Rajab Natshah Jordan
I agree with you Qusai, more flexibility.
In your free time. Let us attach a new patch for 🐛 Fix Drupal 10.3 compatibility for Too few arguments to function deliver() 3 passed in Drimage Needs review too ( the upstream )
and change it to needs review. - 🇯🇴Jordan Qusai Taha Amman
Thanks Rajab!
Sure thing I will update the patch on it ASAP - 🇯🇴Jordan Rajab Natshah Jordan
No longer needing this patch as a fix.
Updated the MR to work with latest 1.0.x-dev
Changed the title and Category of the issue as feature request -
rajab natshah →
committed f9f68b8b on 1.0.x
Issue #3456065: Change to use the $scheme variable for more flexibility...
-
rajab natshah →
committed f9f68b8b on 1.0.x
Automatically closed - issue fixed for 2 weeks with no activity.