Make core aware of Nginx and PHP-FPM to avoid confusing alerts

Thanks for the fix.

There's no patch here, so marking needs work. If you'd like to try your hand at one, see instructions at http://drupal.org/node/707484. We also fix issues in D8 first, then backport, so bumping the version number.

til now, Drupal only supports Apache & IIS officially.

more important is getting PHP5.4 uploadprogress to CORE.

added: #1561866: Add support for built-in PHP session upload progress

Re: #7, I was able to apply the patch in D8. Nothing blew up. :D

But I also need the following to test, right?

Nginx upload progress module: http://wiki.nginx.org/NginxHttpUploadProgressModule
Drupal FileField Nginx module: http://drupal.org/project/filefield_nginx_progress

But it seems like the Drupal module is not available yet for D8. Any suggestions on how to test?

patch should fix the things without extra module.

Any news on this?

Thank you

#7: drupal.nginx-upload-support-1559116-D8.patch queued for re-testing.

This is a bug since #1839336: Add Nginx to INSTALL.txt and recommend it alongside Apache, but it will need a reroll.
The patch is logically correct.

I'm attaching more complete patch for Drupal 7, for now. I will then port it to D6 and D8.

Here is a D6 port, which will obviously fail in testing, but it works.

13: 1508-Issue-1559116-by-mva.name-omega8cc-Make-core-aware-o.patch queued for re-testing.

Attached is a port to D8 for review.

I've applied patch in #13 in my D7 site and the following notice has appeared:

Notice: Undefined variable: is_apache in file_requirements() (line 73 of /var/www/mysite/modules/file/file.install).

I'm running php 5.4.21 on nginx/1.4.3

My "upload progress" status report has changed from

Your server is not capable of displaying file upload progress. File upload progress requires an Apache server running PHP with mod_php.

to

Your server is capable of displaying file upload progress through APC, but it is not enabled. Add apc.rfc1867 = 1 to your php.ini configuration. Alternatively, it is recommended to use PECL uploadprogress, which supports more than one simultaneous upload.

Thank you for working on this

Interesting, I have tested it with PHP 5.3 only, but with PHP notices disabled. Note that both D7 and D8 patches passed all tests on d.o with flying colors above.

Ah, found it. Will submit the patches with the missing $is_apache = FALSE; line ASAP.

Updated D7 variant for review.

I think we no longer use $_SERVER, use $request instead. For example: \Drupal::request()->server->get('SERVER_SOFTWARE')

Shared code could also be refactored.

@jcisio - not exactly. Depending on the context D8 still uses both the old $_SERVER method and the request method, like here:

/**
 * Detects whether the current script is running in a command-line environment.
 */
function drupal_is_cli() {
  return (!isset($_SERVER['SERVER_SOFTWARE']) && (php_sapi_name() == 'cli' || (is_numeric($_SERVER['argc']) && $_SERVER['argc'] > 0)));
}

But of course I can convert the patch to use only the request method (it does use it in file_requirements()) - just not sure if this is a good idea in this particular context, since it seems that what we need is very similar to the logic and method used in drupal_is_cli() test.

#23 those are very old code. New code should follow the standards: https://drupal.org/node/2150267.

But this is the *current* code in D8 (head).

Better patch for D6 for review.

omega8cc, though I'm an Nginx fan and think it's important that core acknowledges the existence of other server daemons, I disagree with the direction you're taking in regards to not creating the .htaccess files when a non-Apache daemon is in use. For example, my employer's sites are hosted on Apache, but I personally use Nginx locally for development. With this patch, the .htaccess file would not be created for me locally, and then would not be present if I, say, use rsync or scp to copy the files directory from my local machine to the dev one. It does no harm to still create .htaccess files when Nginx (or IIS or…) is in use, and will do a lot of good in multi-platform environments.

Refactored patch for D8 for review attached.

@Garrett Albright

It is not about being a "fan". It is about common sense and getting rid of Apache-centric thinking in core. Creating and managing .htaccess is a waste of resources and a source of confusion for any Drupal site hosted on Nginx. It is much more important to get rid of it and its horrifying false errors/warnings and insisting that you can't use perfectly supported features, than maintaining cross-platform compatibility, in my opinion.

Also, I consider it a major developer failure if you develop a site on a different environment than the live environment where the site is hosted. I don't think the core role is to help you continue avoiding standard best practices.

But feel free to disagree and propose better patches for D6, D7 and D8.

omega8cc;

You make it sound like Drupal shouldn't be dealing with .htaccess files at all - and though I agree that that would be ideal, it's not realistic while Apache and .htaccess files are still so prominent on shared hosts and such.

But that's okay. I'd rather have the vast minority of non-Apache users (who are likely to be more technically savvy anyway) be potentially confused by .htaccess files that don't do anything than Apache users be inconvenienced by having to create .htaccess files manually or whatever.

As I mentioned here, I think the real problem - and one not unique to Nginx, as it will happen on Apache too - is that core is incapable of correctly regenerating the .htaccess files on already-installed sites; *everyone* who did an upgrade saw those horrifying errors. That should be fixed directly, though, not worked around for non-Apache daemons (though now that it already shipped this way, prematurely, perhaps it's too late).

I think the only thing that really needs to be fixed is the lie that upload progress doesn't work if you're not using Apache, but since I don't use upload progress anyway, it doesn't bother me that much.

As for it being a failure to have different dev and live environments, that's just silly. What about newbies who build their sites on their cheap Windows desktops and then upload them to cheap Linux shared hosts? Drupal, like the web itself, should be as widely accessible as sanely possible.

@Garrett Albright

You are blurring the problem here by adding irrelevant remarks about Drupal's failure to update .htaccess on Apache powered sites. This should be discussed elsewhere.

I should also mention that my position here is not based on any personal bias or preferences. It is based on direct experience with hosting thousands of Drupal sites on Nginx for many years. We at BOA and Omega8.cc can continue to patch the core to make it Nginx friendly, but I would love to see core aware of non-Apache web servers existence and popularity finally, instead.

Modified patch for D8 again.

Let's test the D8 patch.

This time it should work for D8.

Doh! Maybe someone smarter than me will be able to convert the original working D8 patch properly.

37: nginx-php-fpm-compatibility-1559116-37-D8.patch queued for re-testing.

OK, so all patches for D6, D7 and D8 passed all tests.

My D6 reports page thanks you @omega8cc. the patch in #26 worked.

Patch in #21 works for D7. Also we tested #23 for D6 & #37 for D8. Thanks omega8cc.
+1 for RTBC

Re-roll.

Can we encapsulate the detection logic inside a function like server_supports_htaccess, it will make the code cleaner

Big +1 for this

This is not RTBC, as there is no working patch. Please read https://drupal.org/node/156119 to learn what the statuses mean and when to use them.

re-roll of #45.

some fixes and code refactor

@droplet You have "simplified" the if/else logic in the web server identity check we have added on purpose: for backward compatibility with Nginx based systems which had to use fake web server identity (like Nginx/Apache) to force Drupal core cooperation. Don't remove this, because it doesn't fix the existing problem.

I would recommend that any updates shouldn't try to "improve" the logic we have added and tested in #37. Just reflect any changes in the D8 core after #37 was tested and confirmed as working properly.

Two and a half years...

Got to be the longest anyone has waited for a progress bar.

OK, so let's move the htaccess debate to a separate issue and get the original issue fixed here at least for Drupal 8. Simplified patch attached for review.

If anyone is interested in a fix for Drupal 7, please use the patch from #21.

For Drupal 6 / Pressflow 6, please use the patch from #26

Thank you!

I reviewed and tested manually the patch and it works as expected.

I agree on moving the htaccess debate to a separate issue.

re #55 nginx is currently displaying a progress bar - no is waiting for that. All people are waiting for is an incorrect warning to go away.

The issue summary reads more like an email than an issue summary. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary. AFAICS see this is allowable because it is fixing a bug with no disruption.

I hope that the updated summary is correct! Please review.

Reading the summary is not clear if a working patch for D7 exists, and what nginx modules/settings someone have to install after patching Drupal to get an upload progress bar.

This could be very useful to many looking for a working progress bar with nginx.

Thank you

@MXT

This issue is not about Nginx/PHP-FPM configuration to make upload progress working. It would be a contrib space and server configuration docs which don't belong here.

As for versions other than D8, please see my comment #58 above -- feel free to update the issue summary, but keep in mind that we have made a decision to make the D8 patch reviewed and (hopefully) committed first.

Here is a working example with Nginx, PHP-FPM and Drupal 7:

Steps to reproduce:

1. Install Aegir BOA stack on any VPS: https://github.com/omega8cc/boa
2. Install D7 site
3. Enable the module: https://www.drupal.org/project/filefield_nginx_progress

Done!

The summary looks good to me (I'm RTBCing it now) and the patch in #58 was already RTBCed.

Committed b4494af and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

This looks good for a backport.

Here is a Drupal 7 backport of #69.

Oops, just saw the existing patch for Drupal 7 (#21) (which still applies cleanly, BTW). Please disregard #70.

@bugsonly you are right that should probably be taken care of but it's not the code this patch was touching.

Could you open a new follow-up issue for #73. The error you are reporting is due to file_save_htaccess(), and may be permissions related problem actually because I think we are saving that .htaccess file regardless.

This issue is about install alerts and that is file saving permissions related.

FWIW, we have been using the changes from #70 in Backdrop since Jan 2015 (see https://github.com/backdrop/backdrop/commit/00c42c556358510d6fa9eabdef28...), and there have been no issues reported. As such, I'm marking this as RTBC.

@klonos the policy on how backports to D7 is handled has changed over the years. The current policy is to open a new issue and close the Drupal 8 / 9 issue.

One thing we do need is a new test run against D7 - in the unlikely event the old patch causes a test fail. I'll ping the current D7 maintainers about this one.

Please could someone file a new issue for D7? Adding that to #3216978: [meta] Priorities for 2021-12-01 release of Drupal 7 would give it the best chance of being looked at for the next release. Thanks!

I have created the D7 backport issue: #3266018: Backport Make core aware of Nginx and PHP-FPM to D7

This issue can be closed and for D7 we can continue there.

Marking as fixed from 80