Remove Singleton hack in registerWithSymfonyGuesser

Thank you for creating the issue!

The hack actually broke compatibility with HHVM's repo authoritative mode (i.e. their production-mode "no new code allowed" rules) because they don't allow Reflection to change accessibility in this mode:

Fatal error: We've already made many assumptions about private variables. You can't change accessibility in Whole Program mode in /tmp/hhvm-nginx/drupal/core/lib/Drupal/Core/File/MimeType/MimeTypeGuesser.php on line 111

This is why we better try to remove the hack and update symfony ...

https://twitter.com/symfony/status/620658883716075520 should make it easier so we don't have to track a commit. After that is done I can roll a patch.

Within hours... #2504967: Upgrade to Symfony 2.7.1 oh well.

2.7.2 excluding this mess. #2531796: Fix hiddeninput.exe in symfony/console. Does not contain the static fix because... I'm actually not sure what needs to be done.

nm, that was pretty straightforward.

I am thinking this should be separate issues, so that review is easier (or at least separate patches?)

It's fine as it is because it's not a critical so no hurry.

@jibran: This issue is not critical, but the Symfony update may be. I see there are a few security fixes from the changelog and we could stay up to date. This issue can then just deal with removing the hack and fix the tests (and not worry about a 200 kb patch file with unrelated, but necessary changes).

Fixing the test. To review the actual changes pertinent to this issue, see the interdiff in this comment and the interdiff in #10. This is the reason I was asking for a different issue. Should we still create one?

hack-fix just has the fix. but really the previous interdiff covered it.

Fix tests. They where holding on to the instance but upstream unsets the instance not the property.

@neclimdul: I think you deleted my patch in #15 by mistake (instead of hiding them?). That was the fix for the failing tests.

I am just uploading it again for review. It is essentially the same fix but slightly different. Same filenames as in #15.

That's weird. It shows green but there was a random failure. The same patch in test job in #15 shows all tests passed. So, I think the patch is good for review.

Looks fine for me!

Is it related?
Drupal\system\Tests\System\UncaughtExceptionTest 27 passes 4 exceptions

+++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeGuesser.php
@@ -100,21 +100,9 @@ protected function sortGuessers() {
+    // Reset state, so we store more and more services during test runs.

This comment looks incorrect... I don't think we want to store more and more services during test runs :)

re #22 nope - thats #2532434: UncaughtException test doesn't quite uncatch everything

Woops

hm... weird. I didn't pull before diff'ing so I guess there was a conflict. Here is a rebase, no change.

The conflict #2392559: Remove all uses of file_stream_wrapper_get_* and file_get_stream_wrappers

Directly diffing the patches:

 diff --git a/core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php b/core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php
-index 154c3ec..70287bf 100644
+index 9a712a6..6e3f0e8 100644
 --- a/core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php
 +++ b/core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php
-@@ -36,14 +36,18 @@ public function testSymfonyGuesserRegistration() {
+@@ -37,14 +37,18 @@ public function testSymfonyGuesserRegistration() {
      $container = new ContainerBuilder();
-     $container->set('file.mime_type.guesser', new MimeTypeGuesser());
+     $container->set('file.mime_type.guesser', new MimeTypeGuesser(new StreamWrapperManager()));
      MimeTypeGuesser::registerWithSymfonyGuesser($container);
 +    $symfony_guesser = SymfonyMimeTypeGuesser::getInstance();
      $guessers = $this->readAttribute($symfony_guesser, 'guessers');
@@ -452,7 +452,7 @@ index 154c3ec..70287bf 100644
      $count = count($guessers);
  
      $container = new ContainerBuilder();
-     $container->set('file.mime_type.guesser', new MimeTypeGuesser());
+     $container->set('file.mime_type.guesser', new MimeTypeGuesser(new StreamWrapperManager()));
      MimeTypeGuesser::registerWithSymfonyGuesser($container);
 +    $symfony_guesser = SymfonyMimeTypeGuesser::getInstance();
      $guessers = $this->readAttribute($symfony_guesser, 'guessers');

Back to RTBC.

+++ b/core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php
@@ -37,14 +37,18 @@ public function testSymfonyGuesserRegistration() {
+    $symfony_guesser = SymfonyMimeTypeGuesser::getInstance();

Should we add a comment explaining why we are calling getInstance() repeatedly?

In that vein, it seems like a code smell that a method called registerWithSymfonyGuesser also resets the SymfonyMimeTypeGuesser. I would even suggest renaming that method (but that could be in a follow-up, this patch contains Symfony updates as well).

PS I still think it would have been a good idea to split the Symfony update in another issue so that this could have been a simpler patch to review. I have created #2533360: Upgrade to Symfony 2.7.2. Feel free to close this one if you think this is done anyway.

If #2533360: Upgrade to Symfony 2.7.2 is handled separately, the patch in #26 becomes this.

EDIT: Hopefully, this is easier to review.

#2533360: Upgrade to Symfony 2.7.2 is in. I am reuploading the patch in #30 (which is basically the one I derived from #26) for review. My concerns in #29 still stand:

In that vein, it seems like a code smell that a method called registerWithSymfonyGuesser also resets the SymfonyMimeTypeGuesser. I would even suggest renaming that method.

Since the patch is essentially the same, I am leaving it at RTBC although I'd still like to consider the point above.

#32, okay. I was concerned with something in contrib calling that method. But if it is only early in the kernel bootstrap, it should be fine. However, if for some reason we boot kernel within a kernel, and a contrib is using that, this would be a problem. A very edge case scenario, I know.

I don't think we need to document why we are calling things multiple times in the test.

Fixing title

Committed 2873e81 and pushed to 8.0.x. Thanks!