Remove error “Public files directory Not fully protected” in non apache servers

Simple working patch

Simple working patch

@pankaj1390 Why did you remove the comment line from the patch in #20? Your code in #23 and #24 is identical but you removed the documentation comment with no explanation.

I made an MR based on the patch in #20 with a few updates to the language (add a comma, remove an extraneous "the").

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Moving to NW for the issue summary update requested in #9

Also as a bug this will need a test case.

Why this is not include in core because number of user using ngnix?

@pankaj1390 Please read the issue priority guidelines → and do not change the priority without a valid reason.

Also bugfixes always go into the working development version of Drupal, which is currently 10.1.

Updating the issue summary and removing the tag.

5 years, and this remains obstructed with astonishingly unclear prescriptions as to why.

It is no wonder folks are reluctant to contribute.

@emjayess The remaining task before this issue can be marked "Needs review" is to add a test as stated in smustgrave's comment 🐛 Remove error “Public files directory Not fully protected” in non apache servers Needs review .

@ptmkenny:
system.install @ system_requirements() is 1.5k lines of procedural goo – not exactly lending itself to a unit test. Further, the block emitting this apache-only error to apache and non-apache systems alike, is a system test.

If the hang-up is that this block of twenty lines of (test) code requires additional test coverage elsewhere in a test suite, then it would follow that a test supplying that coverage would pre-exist. I cannot find one. If someone could chime in on the rationale or logic for a test of this test, and where that test needs to live, that would be swell.

As this is merely a "bug" of omission in the first place – since system_requirements() does in fact test the server $software elsewhere for apache, but for some reason (of omission/oversight) not here – it seems to me to be an indication that there is no test coverage for this code now. Probably because it is a system test in and of itself.

Please also note that system.install @ system_requirements() around L219 runs another test for clean url (rewrite module) support, and in fact does already only conditionally proceed for apache webservers, and it conditionally does so as follows:

> && str_contains($software, 'Apache')

If there indeed does exist a separate unit or functional test supplying code coverage for this test, please point at it forthwith, as a guide.

If there is not any existing coverage of all this procedural goo, then this apache-only alarmism has been needlessly nagging maintainers of non-apache systems for years, for no good or justifiable reason, because of a bogus objection and obstruction to adding a simple && str_contains($software, 'Apache') condition to an if statement, to bypass the code for non-apache web servers.

Lower quality software lingers on and on due to insistence on what is perceived to result only in higher quality software... clearly isn't working.

The real chore that evidently is being avoided is to boil away this procedural goo and get security checks much better organized somewhere other than inline'd in system_requirements(). Forking procedurally for all these checks across all the possible web servers using if statements in a multi-thousand LoC function is very non-viable and non-testable, and as is self-evident, a maintenance headache.

Thank you for searching for a test. It's quite possible that there is no test.

A little background may be helpful here: at some point in the Drupal 8? development cycle, it was decided that new commits to core require test coverage, so it is the responsibility of new commits to add test coverage if there isn't any already. That's why this issue is stuck.

The reason for requiring tests is to ensure that "fixed" things don't get broken again.

If you want to debate the rationale for this policy, please don't do it in this issue; a better place would probably be the #contrib channel in the Drupal Slack.

Hopefully someone else better informed than me can describe what kind of test should be written for this.