I'm filing this against 8.4.x in hopes that it can get fixed before release. Feel free to move it as appropriate.

The Symfony API change at [#2743809] does not affect core only; it's a breaking change for any module that needs to inspect a file being uploaded. We have custom code that looks like this:

$upload = \Drupal::request()->files->get("files[attachment]", NULL, TRUE);
if (empty($upload)) {
  // Nothing to do here! The user didn't attach a file.
}
else {
  $importantFile= file_get_contents($upload->getPathname());
  $this->sendToThirdPartyAPI($importantFile);
}

8.4 breaks this kind of file handling, invisibly. In the example above, $upload is empty even if the user uploaded a file; no exception is thrown.

CommentFileSizeAuthor
#4 2906030-4.patch8.66 KBdawehner
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

ksenzee created an issue. See original summary.

xjm’s picture

Issue tags: +8.4.0 release notes

Hmm I wonder if there is a way we could fail explicitly.

Meanwhile, added this issue to the CR. If nothing else we should add this to the known issues in the release notes.

cilefen’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
8.66 KB

Missing files is a tough problem. We could potentially even implement the function again. This is quite a golden hammer, but maybe worth doing?

I think we have a couple of possiblitites:

* Triggering a E_USER_DEPRECATED like symfony in 2.8 did: https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/HttpFo...
* Trigger an error without hiding it with @
* Reimplement the function

Here is a concept of doing the later one.

dawehner’s picture

To be fair there has been 3 minor releases between the update to 2.8 and 8.4, so I'm honestly wondering whether we need to somehow adapt our deprecation handling.

xjm’s picture

@dawehner, well, we haven't actually even gotten to the point where we fail deprecated uses in the test suite, have we? So yeah, probably need to get that working so people are informed when they are using a deprecated code path that we have already marked as such.

mpdonadio’s picture

I like the approach in #4. I think it may also be good way to handle some of the other things that are being deprecated in recent Symfony versions (eg, the proxy headers).

dawehner’s picture

@dawehner, well, we haven't actually even gotten to the point where we fail deprecated uses in the test suite, have we?

Yeah we haven't indeed, see #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code, so we should not base any decisions upon that.

Gábor Hojtsy’s picture

I am trying to incorporate this in #2895685: Create release notes (a.k.a CHANGELOG) for Drupal 8.4.0-alpha1 and Drupal 8.4.0 (as a known issue) but having a hard time seeing what is the problem exactly here. Can someone update the issue summary with conditions of when would this happen?

Wim Leers’s picture

The 8.4.0 release is happening tomorrow. The release notes are still pointing to this issue, and a clarified IS like Gábor requested in #9 would still help make the release notes clearer.

(I don't think it's likely that the patch in #4 will land by tomorrow.)

effulgentsia’s picture

Does #4 fix the issue outlined in the issue title and summary?

+++ b/core/lib/Drupal/Core/Http/RequestWithBC.php
@@ -0,0 +1,39 @@
+    $this->files = new FileBag($files);

Looks to me like this is still using the one without $deep.

effulgentsia’s picture

Oh, sorry, never mind. RequestWithBC is in Drupal/Core/Http/, so is loading FileBag from there. I missed that when I wrote #11.

effulgentsia’s picture

According to the CR the usage was deprecated by Symfony by 2.8.

Yes, but it was not deprecated in Symfony 2.7, which is what Drupal 8.0 shipped with. So any modules written against Drupal 8.0 wouldn't have even known about the deprecation.

#2721139: Replace deprecated files ParameterBag usage was fixed for core in Drupal 8.1, so any new contrib modules written from after that time, would be using the new pattern if they were copying from file.module or looking at the Symfony code for the first time. But I don't know if a change notice about it went out (there isn't one linked to that issue specifically), so contrib modules already written before then could have remained unaware about the Symfony deprecation and needing to make the same changes in their code, as per the issue summary.

effulgentsia’s picture

Title: File upload handling invisibly broken » File upload handling invisibly broken within contrib/custom modules that still use the API that was recommended in Symfony 2.7 / Drupal 8.0
effulgentsia’s picture

I don't know if a change notice about it went out

Looks to me like the only notice that went out about Drupal 8.1 / Symfony 2.8 deprecating some stuff is https://www.drupal.org/node/2671022. It doesn't mention anything about parameter bags or file uploads. It links to https://github.com/symfony/symfony/blob/2.8/CHANGELOG-2.8.md, which does mention Deprecate $deep parameter on ParameterBag, but kind of buried in a long list of things, and it doesn't mention anything explicitly about file uploads, so I can definitely see how contrib/custom modules already written against Drupal 8.0 / Symfony 2.7 could have missed that.

dawehner’s picture

so I can definitely see how contrib/custom modules already written against Drupal 8.0 / Symfony 2.7 could have missed that.

So given that do you think its worth applying this fix/or anything which changes the behaviour back? At least for me file bags aren't accessed often, so we don't have to worry about the performance aspects. We could now create a change record explaining that and then maybe deprecate it in 8.5?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.