Add support for built-in PHP session upload progress

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.

yash.rode → made their first commit to this issue’s fork.

I have applied the patch #51 except one hunk where we Randomly generate a valid PHP session identifier 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 📌 Add support for built-in PHP session upload progress Needs work .

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

Can someone tell 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

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

I was trying to implement #85 📌 Add support for built-in PHP session upload progress Needs work . 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 📌 Add support for built-in PHP session upload progress Needs work , I need some help with the existing approach, 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 📌 Add support for built-in PHP session upload progress Needs work . 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 ];
$status = uploadprogress_get_info($_GET['key']);
$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 📌 Add support for built-in PHP session upload progress Needs work . 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 📌 Add support for built-in PHP session upload progress Needs work , 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 ✨ Upload progress using jQuery.form plugin instead of 3rd party PHP libraries Needs work ?

Yes, they are related. Once ✨ Upload progress using jQuery.form plugin instead of 3rd party PHP libraries Needs work 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