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.Problem/Motivation
When loading local pages that call stage file proxy a warning is thrown as NULL is being passed to a trim function.
Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\stage_file_proxy\EventSubscriber\ProxySubscriber->checkFileOrigin() (line 135 of modules/contrib/stage_file_proxy/src/EventSubscriber/ProxySubscriber.php).
Drupal\stage_file_proxy\EventSubscriber\ProxySubscriber->checkFileOrigin(Object, 'kernel.request', Object)
call_user_func(Array, Object, 'kernel.request', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.request') (Line: 134)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 49)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 49)
Steps to reproduce
- Use PHP 8.1
- Use a version of the site connected via stage file proxy
- Load a page with lots of files to get
- Warnings are displayed
Proposed resolution
To prevent warnings check for NULL before passing to trim function.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | interdiff-3293275-2-11.txt | 578 bytes | markdorison |
| #11 | stage_file_proxy-php81-trim-warnings-3293275-11.patch | 769 bytes | markdorison |
| #2 | stage_file_proxy-php81-trim-warnings-3293275-2.patch | 730 bytes | 3li |
Issue fork stage_file_proxy-3293275
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
3liComment #3
3liComment #4
gregglesWhy is the origin dir configured to be null?
Comment #5
albeorte CreditAttribution: albeorte at Metadrop commentedComment #6
markdorisonComment #7
markdorisonWhen the module is first enabled won't that value be empty and return NULL?
Comment #8
gregglesI guess so. If it is, should the module silently ignore the problem or should it be noisy that the configuration needs to be set?
Comment #9
markdorisonIn my opinion, I think it is an expected state that we should account for, rather than a problem.
Comment #10
gregglesYeah, I think we agree it's an expected state we should account for.
As I understand the proposal this change would account for it by silently using the value which will then cause the remote requests to fail.
It seems another way to account for the expected state would be to display an error or log an error that encourages the admin to configure the module.
I don't feel super passionately about this perspective, though, and I'd be fine with this getting committed (though the test failures need to be reviewed).
Comment #11
markdorisonI see what you are driving at. It's that the change in patch #2 is going to change the behavior of the subsequent check on
$remote_file_dir.I am not against this approach but is there a way to complete this issue without changing the functionality and then address a change like that in a separate issue? Would an additional change to our check on
$remote_file_dir(see included patch) resolve the warning while also preserving the existing functionality?Comment #12
smustgrave CreditAttribution: smustgrave at Mobomo commentedJust to chim in we did last night change the 8.x-1.x branch to be D9 only and started a new 2.0.x branch that will be D10 only. In case that plays into this conversation some.
Comment #13
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #16
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded to the 2.0.x branch!