Add support for built-in PHP session upload progress

Suppose this functionality should be pluggable to support http://wiki.nginx.org/HttpUploadProgressModule

There's a project http://drupal.org/project/filefield_nginx_progress

This should work with nginx with php-fpm no?

I understand that there are also problems with php-fpm & PECL’s uploadprogress.

As people keep looking for new ways to enhance performance, it would be good to be able to leverage tools built into PHP 5.4+

I played around with PHP Session upload using nginx module as a base. Very quickly I worked out that PHP Session Uploads Don't work at all for CGI/FastCGI installs.

I read that you can switch to mod_fastcgi and using the -flush command you can get it working.

I am not in control of my clients hosting making this something I cant use.

Attached is my D7 test module that started out as a copy of nginx module. I never got it working but somebody else may find it helpful as a starting point.

As I can see on function page:

Note, this feature doesn't work, when your webserver is runnig PHP via FastCGI. There will be no progress informations in the session array.
Unfortunately PHP gets the data only after the upload is completed and can't show any progress.

Note, this feature doesn't work, when your webserver is runnig PHP via FastCGI. There will be no progress informations in the session array.
Unfortunately PHP gets the data only after the upload is completed and can't show any progress.

Has anyone tried with Nginx >=1.7.11 and fastcgi_request_buffering set to "off"? This might be exactly what's needed. Of course it means a server level change for the locations where the POST is going to happen though so not ideal for everyone, but may at least allow the feature to be used by advanced users with server-level access.

http://nginx.org/en/docs/http/ngx_http_fastcgi_module.html#fastcgi_reque...

Now that PHP 5.5 is the minimum required version all systems should have Session Upload Progress built in so should make the patch simpler.

Also, the status report:

Your server is not capable of displaying file upload progress. File upload progress requires PHP be run with mod_php or PHP-FPM and not as FastCGI.

is incorrect. Does that mean this issue should be classed as bug rather than a feature request?

Am I correct in thinking that this mis-reporting occurs on every system? As it is so widespread should this be a major?

I believe there is a case to be made that Drupal 8 should get rid of APC rfc 1867 and Uploadprogress support altogether and replace it with PHP's native Session Upload Progress.

Reasons to support the case:

• both APC and Uploaprogress require separate PHP extensions to be installed
• APC rfc1867 does not support multiple file upload
• Uploadprogress has not been worked on forever. The latest version 1.0.3.1 is from 2011
• Both APC and Uploadprogress do not work with PHP 7 correctly. FileWidgetAjaxController::progress is using accordingly apcu_fetch or uploadprogress_get_info()that break PHP parser causing script to stop working and thus rendering error like:
  
  

  An AJAX HTTP request terminated abnormally.
  Debugging information follows.
  Path: /file/progress/706540310
  StatusText: error
  ResponseText: 
  ReadyState: 0
      

  for AJAX callback for Drupal::ProgressBar::sendPing()

• Allegedly there is a repo for Uploadprogressextension https://github.com/Jan-E/uploadprogress that is a port to PHP 7 but I was not able to make it work. Failed with the same error as reported above.
• When Uploadprogress extension is enabled it interferes with Session Upload Progress making it unusable whereby $_SESSION["upload_progress_xxx"] array is never created by PHP.

Bottom line going with PHP native solution will simplify code, and remove dependencies on PHP extensions.

Bottom line going with PHP native solution will simplify code, and remove dependencies on PHP extensions.

This makes complete sense to me. Since D8 now requires php >= 5.5, this will not be a php-version issue at all.

Moving back to 'active' since there is no actual patch here yet.

An added bonus:

The web server's request buffering has to be disabled for this to work properly, else PHP may see the file upload only once fully uploaded. Servers such as Nginx are known to buffer larger requests.

This is also a requirement for the Bigpipe module to function properly, so folks that have that working will immediately benefit from this change.

It was already mentioned the uploadprogress doesn't have a release in PECL that supports PHP7. It also appears that rfc1867 support was not included in APCu 5 (https://github.com/krakjoe/apcu/issues/212), so PHP's native upload progress handling looks like the only option available for PHP 7.

Hi @droplet

Thanks for the pointers about jQuery.form. I've also tried to get the built in PHP session upload progress to work, with no success.
I've created a separate issue with a patch that implements the jQuery.form uploadProgress function.

https://www.drupal.org/node/2833968

Also there's another way to offload php https://www.nginx.com/resources/wiki/modules/upload_progress/

As mentioned in comment #17, using a custom session handler breaks the built-in PHP session upload progress extension. The attached patch temporarily reverts to the built-in PHP session handler and retrieves the upload progress data. It then restores Drupal’s custom session handler. This only works if PHP is configured to use the same cookie name as Drupal and if no other upload progress extensions are enabled. The fix for #2662932: Fix file upload progress bar must be applied before this can be tested.

Fixed failed tests.

Fixed another failed test.

In case anyone else wonders if switching between session handlers is necessary, I just tried setting the default session.save_handler to user. I thought PHP might hold on to the session data until Drupal started its session manager, but PHP just crashed.

+1 for patch #30.

1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -273,6 +284,10 @@ public function setWriteSafeHandler(WriteSafeSessionHandlerInterface $handler) {
   +  public function getPHPSessionHandler() {
   

   This needs to have a docblock.

2. +++ b/core/modules/file/file.es6.js
   @@ -222,8 +222,9 @@
   +        // Replace the name with the last string in the name that does not
   +        // contain square brackets.
   +        $progressId.attr('name', originalName.match(/[^\[\]]+(?!.*[^\[\]]+)/)[0]);
   

   I agree that it's a good thing to document what this regex does. I would like to know the why of this regex though. Why are we doing this regex.

   So something like this:

   Replace the name of progress to an identifier that doesn't contain square brackets because that breaks ...
   

3. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
   @@ -39,6 +71,41 @@ public function progress($key) {
   +      // Save the current session handler and data.
   +      $save_handler = session_module_name();
   +      $session = $_SESSION;
   

   This comment doesn't really explain anything. It can be removed.

4. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
   @@ -39,6 +71,41 @@ public function progress($key) {
   +      // Stop the current session without saving.
   +      session_abort();
   

   This comment doesn't explain the why. So either remove, or better, explain why we need to do this.

Fixed points 3 and 4 mentioned in #33.

Here is the patch with all documentation fixes added.

In testing this locally I managed to get the upload progress widget indicator to show up just fine but it seems to be stalled until the upload completes. I did try with very large files but it didn't make any difference. Simplytest.me seems broken ATM so I couldn't test there to rule out any local setup oddity.

Has anyone been able to get this working correctly with the latest patch?

The upload progress bar is broken by #2662932: Fix file upload progress bar. The patch for that issue has to be applied before testing this one.

Added check for correct PHP session name to status report.

Thanks — the combination of #2662932-74 and #1561866-38 is working well for me so far.

While testing this patch, I had a session_id() that happened to contain an underscore. php_session_valid_key() returns false if it contains an underscore, so PHP wasn't saving the session and upload progress wasn't being reported.

So this issue also depends on #2238561: Use the default PHP session ID instead of generating a custom one or another fix for the invalid-underscore-in-session_id issue (such as doing a simple str_replace to convert the underscores into commas, as in the attached patch).

Also, regarding the status report message:

$description = t('The <a href=":url">PHP session name</a> must be set to %session_name before Drupal starts in order to store file upload progress. It is currently set to %php_session_name.', [

It might be helpful to be more explicit about how to do this; e.g., change it to something like …must be set to %session_name in php.ini (which is applied before Drupal starts) in order to store file upload progress…

session.name can be set in php.ini, .htaccess, httpd.conf or .user.ini. If the fix for #2455465: Add mod_php7 check to htaccess and remove php5 code were committed, I would recommend setting session.name in .htaccess. Until that happens, I recommend that we refer users to the PHP documentation so they can make the right decision for their situation.

Here is a new patch that

1. Skips checking the server PHP is running on if the PHP session upload progress extension is being used.
2. Adds instructions for setting the PHP session name to match the Drupal session name, if we can determine what method the server supports.
3. Ensures that the session identifiers Drupal generates are valid PHP session identifiers by using commas instead of underscores in base 64 encoding.

Added coding standards fixes.

Comma encoding in session identifiers was throwing off tests. This patch prevents invalid characters in the session identifier by using base 32 encoding instead of base 64.

Please make sure smokris is included in the commit credit for raising the problem of underscores in session IDs.

I’ve been reading further and it looks like the built-in session upload progress extension may have the same limitations as the others, so leaving the server checks alone makes more sense.

Given that the upload progress doesn't touch PHP thus won't touch Drupal, is there any way to automatically test this? Otherwise, what criteria do we set in order to get this into core?

Apc no longer provide upload and core require php 7.0

Uploadprogress will be released soon with upcoming PHP 8.0 this week

https://github.com/php/pecl-php-uploadprogress/pull/5#issuecomment-73237...

@andypost suggested there might be known issues with this for PHP 8 compatibility at present, so tagging for the release notes to go in the "Known issues" section with an explanation.

The uploadprogress got release 2.0 and only yaml PECL extension still not ready but works with patch

I was checking to make sure this wasn't one of the issues that needed release notes for the upcoming 9.5.0 release, and it isn't. Clarifying that.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I have applied the patch #51 except one hunk where we Randomly generate a valid PHP session identifier and set it. because #2238561: Use the default PHP session ID instead of generating a custom one so I think we don't need that anymore?

I think, Drupal now uses the default session handler instead of creating a custom sessions handler, so removing that part of the code according to #25.

Yes, session handling mostly polished nowadays, except few kinds of tests #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush

Can someone tell me, how can I test this. I am seeing Enabled (PECL uploadprogress) on the status report page, but I am not able to see the progress bar while uploading the file and the documentation seems outdated to me.

It is working for Apache testing for NGINX

The MR is working for me with Apache as well as NGINX, following is the working snapshot with NGINX.

For the IS update did not test.

Sorry, I did not get the last comment.

tagged for issue summary update years ago but still has not happened.

Cleaning up tags and the IS. I think leaving an issue with a working patch at NR is fine, even if there are other "needs" tags. It still needs review.

Would slightly disagree with that. especially if one is for the issue summary to be updated. IS should be clear for reviewers, which is now thanks!

With the MR applied, cached cleared.
I'm on nginx though and when I upload a file I don't see any progress bar just the spinner icon.

@smustgrave which backend are you using after Nginx?
Did you ensure that request buffering is disabled and Nginx has no own uploadprogress extension enabled?
I bet you also have PECL uploadprogress extension enabled if you're using docker image

@yash.rode please provide your configuration from comment #74

I'm sure it need proper docs because there's a lot of a-la-ddev environment exists in a wild and everyone has own build of PHP

Leaving for reviews to figure out who also can't make it work

Added steps to reproduce.

I tested it on both Apache and Nginx. Progress bar is visible after changing form display for image to Bar with progress meter.

Screen Recording 2023-07-14 at 11.32.13 AM.mov

Based on the video (thanks!) in #83 this appears to be working.

I suggest to try a different approach here. Instead of attempting to restart the session mid-request (that's dangerous for many reasons), let's try to collect the upload progress data before the session gets initialized.

• Introduce a stack middleware with a low enough priority such that it runs before the session middleware.
• In the middleware, extract the upload progress data from the $_SESSION superglobal and add it to some request attribute. Immediately save and close the session before passing control to the underlying middleware.
• Modify FileWidgetAjaxController.php to collect the upload progress data from the request attribute.
• Undo all changes to SessionManager.php

A note on the design of this PHP feature. File upload progress data is saved to the session store before the SAPI hands over control to the actual PHP script. It follows that any session store is used which happens to be configured in the execution environment (i.e., via ini settings in any of the configuration files within the search path). Hence, PHP will use whatever session.* ini setting happens to be in effect prior to the initialization of SessionManager (via the Session stack middleware).

Instead of instructing people to change their session.name ini settings, I suggest to just work with the server defaults. This will result in two cookies being used. One for the upload progress (with the name PHPSESSID in most cases) and a different one with the actual Drupal session data. This shouldn't be a problem with the design outlined above.

N.b., calling session_module_name mid-request effectively removes the session_handler service, and as a result any subsequent changes to the session will not be stored to the session record in the database anymore.

Implementing #85

I was trying to implement #85. I have a doubt in that, in \Drupal\Core\StackMiddleware\PreSessionHandler::handle, I am getting $_SESSION as NULL, so I am not able to access the data to set the attribute. @znerol do you have any idea why would this happen, or where am I going wrong?

Thanks for taking this on @yash.rode. I left a review comment.

While working on this I found out that without even the MR/patch the progress bar is already showing, you just need to make sure `Upload progress` is enabled on the status report page. Can someone please confirm this?
I used Apache and nginx (Laravel Valet). So I wonder if this issue is still relevant?

Agree with @yash.rode.

Tested it on ddev for both apache and nginx as it has uploadprogress enabled by default (https://github.com/ddev/ddev/issues/1249), and once we use Bar with progress meter in Manage form display, we can see the progress bar.

This is should be tested with uploadprogress disabled.

Also testing should be done using built-in web-server which is started with core/scripts/drupal quick-start

Assigning it for testing, trying to figure out if we need the block of code we are trying to change(\Drupal\file\Controller\FileWidgetAjaxController::progress's else if part). The progress bar exists without that part of code.

Before I go further with #85, I need some help with the existing approach from #51 which is right now part of MR, where we are stopping the Drupal session and starting a PHP session and then doing the opposite. While doing that it is failing at \Drupal\file\Controller\FileWidgetAjaxController::progress on line 97 with the error ValueError: session_module_name(): Argument #1 ($module) cannot be "user". which is causing the progress bar to not make any progress, can someone help me with this? Thanks in advance.

Thanks @yash.rode for your work.

It looks like the code now tries to terminate and restart the session mid-request. Don't do that. Instead, start the session in the middleware, read out the upload progress data from the $_SESSION superglobal, save it in a request attribute and close the session before passing control to the underlying middleware.

Hi @znerol, I was trying #96. If I start the session and end it in the middleware with session_start() and session_write_close() I am getting access denied on the site because, I guess, the session is getting corrupted somehow.

Thanks @yash.rode. I was able to reproduce the access-denied page when attempting to use an existing browser window for testing. It doesn't happen when starting with a clean browser (i.e., no cookies for the test domain). Hence, please always test in a clean browser. I recommend to use a new private window in order to investigate session related changes.

Regarding the architecture: Attempting to use two separate sessions with two separate session handlers in one request (which is necessary if we want to use session based upload progress) is not trivial. That said, our very best bet is to do that in a middleware which separates the session handler for uploads as much as possible from the rest of the system.

Thanks, it is working when the cache is cleared, but $_SESSION is always empty. What am I missing? I tried putting a conditional break point it, We are never reaching it.

Tried c0b4f0f7 (last commit in MR by @tim.plunkett). For sure it would be perfect if we could further isolate calls into the custom session by doing that on /file/progress/ only. However, it looks like this is provoking invalidation of the actual session again (i.e., you get logged out and the next request will be access-denied).

Looking at the FileWidgetAjaxController I wonder whether it would be possible to replace that with a very simple front controller script which doesn't do anything else than reporting progress. In the ideal case that script wouldn't have any dependency at all to other parts of Drupal. As a result we might be able to completely avoid interaction with the authenticated session.

In order to that, it would be necessary to introduce dedicated front controller scripts for each upload method. The decision which implementation to use would happen whenever the upload element is rendered. I think the front controllers could be as simple as this:

core/modules/file/scripts/upload-progress-pecl.php

$progress = [ 'percentage' = -1 ];
$status = uploadprogress_get_info($_POST['key']);
if (isset($status['bytes_uploaded']) && !empty($status['bytes_total'])) {
  $progress['percentage'] = round(100 * $status['bytes_uploaded'] / $status['bytes_total']);
}
header('Content-Type: application/json');
echo(json_encode($progress));

core/modules/file/scripts/upload-progress-session.php

$progress = [ 'percentage' = -1 ];
$key = ini_get("session.upload_progress.prefix") . $_POST[ini_get("session.upload_progress.name")];

session_start();
$status = $_SESSION[$key];
if (isset($status['bytes_processed']) && !empty($status['content_length'])) {
  $progress['percentage'] = round(100 * $status['bytes_processed'] / $status['content_length']);
}
header('Content-Type: application/json');
echo(json_encode($progress));

Please note that these are only ideas. I haven't tested any of the implementations. We have custom front controllers for other cases as well (e.g., core/modules/statistics/statistics.php). But honestly I'm not sure whether it is wise to replicate that pattern.

This could also be explored in contrib if there was a way to alter the front controller script where the upload widget is polling for the progress.

I tried the approach described in #100. I am facing the same problem we had with the previous approach-- $_SESSION is empty. Is there something I am missing, can someone have a look?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thanks for the review, but all I am trying to say is #101 , So basically this approach is not working.

Ah yes sorry, I would try to make it an actual route instead of a single php file for now. If that works we can move on to making the call "lighter" by bypassing some of the Drupal initialization stuff. If that doesn't work we have another issue :)

Which was probably already attempted earlier. I just keep tripping on myself there heh.

Took a look at the front controller approach (c093cd7c). I observed that the post request to session-upload-progress.php is empty. However, the PHP docs state that:

The upload progress will be available in the $_SESSION superglobal when an upload is in progress, and when POSTing a variable of the same name as the session.upload_progress.name INI setting is set to.

I guess that the JavaScript part is incomplete in c093cd7c.

Hi, I tried sending the data, still $_SESSION is empty.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

There should be a variable with the name set to the value of session.upload_progress.name. E.g., in my case i find that session.upload_progress.name has the value PHP_SESSION_UPLOAD_PROGRESS.

$ php -i | grep session.upload_progress.name
session.upload_progress.name => PHP_SESSION_UPLOAD_PROGRESS => PHP_SESSION_UPLOAD_PROGRESS

Hence, there should be a POST variable on every file related request with the name PHP_SESSION_UPLOAD_PROGRESS.

We already have that variable

So in this case we are posting 1202650292.

If I understand the PHP docs correctly, the POST variable needs to be sent by the initial post and in addition with every XHR request. Also the input highlighted in the screenshot actually shows that the POST variable doesn't have the correct name. Hint: You need to have <input name="PHP_SESSION_UPLOAD_PROGRESS"> somewhere in the source.

@yash.rode helped me get set up today and I'm also having the same issue where despite calling session_start(), the superglobal $_SESSION doesn't exist.

It definitely exists when putting a breakpoint in a random place like the node form, and it's full of Symfony variables...

Not sure what is missing here.

I have managed to change the name to what we have in php.ini, but still the $_SESSION is empty.

I was trying to implement a minimal demo in pure PHP (i.e, no Drupal, no Frameworks) but haven't had any success yet. If somebody has a POC, please upload it somewhere and post a link here.

The SO post How to make PHP upload_progress SESSION work? has one good answer. It might serve as a starting point.

https://github.com/TychaK/Track-Upload-Progress-using-Session-Upload-Pro...
this is one of the implementation that I have been referring to. For them $_SESSION is available, and it is not available for us, that's the problem. You can try this implementation as a POC.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Will stop the bot from running on this while you discuss

@znerol would be great to get your thoughts on the PoC that Yash linked to in #117

@znerol just following up if you had a chance to take a look?

Seems this one stalled some. The MR is no longer mergable though so moving to NW due to that.

Is this related to https://www.drupal.org/project/drupal/issues/2833968?

Yes, they are related. Once #2833968: Upload progress using jQuery.form plugin instead of 3rd party PHP libraries is fixed, this problem will also be resolved. The PHP session approach is not working, so we will have to go with the jQuery.form plugin approach discussed in the other issue.

@yash.rode would you say this is duplicate now?

Can move over credit if so