Update Drupal's dependencies

Looks like there are API changes in the new versions.

I traced down the problem to this http://pastie.org/4349738 hunk in Response.php which Crell points out to come from #1651010-23: Unification of drupal_exit() and drupal_page_footer() and performance boost for FPM enabled environments. I will try to see what I can do.

Here. Symfony send now always closes the request and so we can't use ob_get_contents() in drupal_page_set_cache(). But, we have access to it so I have changed RequestCloseSubscriber::onTerminate to send it: if ($config->get('cache') && ($cache = drupal_page_set_cache($event->getResponse()->getContent()))) and drupal_page_set_cache to take a $body instead of calling ob_get_contents.

I accidentally left in the test commenting out I worked on. Here's an interdiff now.

Edit: the change , as seen in interdiff is moving code doing headers/output from TERMINATE into RESPONSE -- this is an actual bugfix because already the Symfony we use closes the connection in send(). It's just more visible now.

I have no idea what is going with the page cache testing. Here's drupal_serve_page_from_cache:

  $page_compression = $config->get('page_compression') && extension_loaded('zlib');  
  if ($page_compression) {
    header('Vary: Accept-Encoding', FALSE);

Here's the page cache test:

$this->assertEqual($this->drupalGetHeader('Vary'), 'Cookie,Accept-Encoding', t('Vary header was sent.'));

There is no other mention of Accept-Encoding in core. None. So how can this test pass if page_compression is off? It certainly fails for me. So here are two patches. The b version is necessary for me that HEAD to pass and again I can't see how would it pass for anyone else.

Edit2: how can a change in PageCacheTest (see interdiff_b.txt for more) lead to 3 more exceptions in UpdateCoreTest? I have my doubts about the reliability of our testing.

The test failures indicate that D8 is currently incompatible with the latest Symfony release. Clearly, we can't ship in that state, so upping this to critical. Also, if the solution requires changes to Symfony, better to figure that out and let Symfony know that before it moves out of beta.

is it possible to separate the doctrine upgrade from the Symfony upgrade? If not, fine, but thought I'd ask.

Eclipse

The issue is not that D8 incompatible with Symfony 2.3 , the fixes so far are fixing valid bugs with Symfony 2.2 running under FastCGI. 2.3 merely made them visible in mod_php by closing the output in send() for non-fastcgi too.

I have fairly quickly found: if you comment out line 80 in StreamedResponse.php $this->setProtocolVersion('1.1'); then file_transfer begins to work all of a sudden. This makes the update tests happier. Adding Content-Length made things better, adding Connection Close made things as fast as HTTP 1.0.

Edit: reproduction instructions: drush si -y minimal; drush en -y update_test; drush ev 'variable_set("update_test_xml_map", array("drupal" => "0"));' printf "GET /d8/update-test/drupal/8.x HTTP/1.0\nHost: localhost\n\n" | netcat localhost 80 observe a 473 before the open tag and a 0 after. I am unable to observe this w/o checking the raw HTTP response with netcat. cURL won't cut it.

I have filed https://github.com/symfony/symfony/issues/5173 because the chunked Transfer-Encoding is actually a Symfony bug.

So this is interdiffed against #8 because it is an upstream bug and so #15 can safely be ignored. I will fix the last two test cases momentarily. For now, yes, I patched Symfony just like that.

Edit: The html.tpl.php html and body attributes fails because theme_test_exit does a print which appears before any other output and breaks the HTML/XML parser. This, despite hook_exit says already "This hook MUST NOT print anything".

This should fix at least the theme test. It is the test that's broken not Drupal because it prints on exit. However, as hook_exit must not print, I moved it back into the TERMINATE subscriber (this fixed the 255 chars report in statistics for me at least). The actual bugfixes are in the review me patch. They are as follows:

1. pass the response body to drupal_page_set_cache instead of futzing with ob (yes! we can inject into functions too!)
2. move the drupal_page_set_cache call into RESPONSE from TERMINATE because TERMINATE can not change output
3. fix the theme tests so they don't try to print in hook_exit
4. fix Symfony's StreamResponse broken HTTP 1.0 comparison

.

The upstream bugfix has been commited already, what does that mean for this issue, postpone until the next version is out?

Nah, we will just commit that once it's out. No problems IMO patching in core during the thaw.

Here's Symfony v2.1.0-RC1 and Doctrine 2.3.0-RC1 No need for upstream patch now otherwise see #21 for the actual fixes.

Opsie. b/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/StreamedResponse.php.rej sneaked in. I now ran grep -A1 dev/null 1698108_25.patch |grep b/|grep -v '.php$' to verify no other such thing.

This should fail as I have added a few asserts to the page caching tests to see that the page is not served twice. Then I will add the fix which is quite trivial.

Edit: apparently #25 missed composer.json and composer.lock for the new Doctrine.

OK, so this issue revealed page caching tests with the bot are unreliable: they passed previously when they couldn't possibly have (#9, #26) and passed all other times when they fail on my machine without any patching whatsoever (again see #9). Nonetheless, here are the reproduction instructions should someone try to test this locally: apply #26, enable page caching, logout, go to home page, page down to see it twice. Only pages not served from cache will do this.

Regardless of what the bot does, this is now ready: the problems revealed by the upgrades were fixed (they were existing problems, just less visible -- just try an unpatched Drupal 8 on FastCGI, most of the test fails from the op will be there), the regression introduced was caught by my manual testing and fixed in the patch attached by deleting the response printed by drupal_serve_page_from_cache.

Tried this locally and can confirm that #26 does have this issue of uncached pages displaying twice, but #27 does not. I have not looked at the specifics of this issue at all, just verifying the behavior described by chx.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.phpundefined
@@ -66,8 +66,18 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
+      $response->setContent('');
+      $response->headers->remove('cache-control');

I wonder would it make sense to add a todo here about switching to Symfony's HttpCache, per #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache? These lines look to me like a temporary solution to get around the way drupal_serve_page_from_cache() works.

+++ b/core/lib/Drupal/Core/EventSubscriber/RequestCloseSubscriber.phpundefined
@@ -28,32 +28,7 @@ class RequestCloseSubscriber implements EventSubscriberInterface {
     _registry_check_code(REGISTRY_WRITE_LOOKUP_CACHE);
     drupal_cache_system_paths();
     module_implements_write_cache();

Is there a case to me made for moving all of the caching into FinishResponseSubscriber? There was talk of moving the caching of system paths forward so that it happens within the scope of the request (because we are storing the system path as an attribute of the Request object, but then don't have access to that during kernel.terminate).

I guess I'm just wondering, is there some reason why caching, in general, really *ought* to be happening during kernel.terminate as opposed to the end of kernel.response? Does it impact performance in some way? If not, why not keep all the caching stuff together in the FinishResponseSubscriber? This would solve a problem for #1599108: Allow modules to register services and subscriber services (events). I'm just using this opportunity to ask the question but it may not make sense to add that change to this particular patch.

send() works truly hard to finish the response (explicit close for FastCGI and flush for non-fastcgi) and all those expensive operations during terminate can happen while the user marvels at your wonderful website. We can't do that for the page caching itself because that's an output changing operation. I see no reason to add a todo to rip this out when it will be ripped out anyways when we convert to HttpCache just by the nature of the beast.

Re doing caching operations during kernel.terminate, that makes sense. We'll need to figure out a different solution for the path alias caching. And yes, I see now that it makes no sense to add a todo regarding the switch to HttpCache since the HttpCache patch will need to rip it out anyway. So I think this is good to go.

+++ b/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Debug/ExceptionHandler.php
diff --git a/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/DependencyInjection/ConfigurableExtension.php b/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/DependencyInjection/ConfigurableExtension.php
old mode 100644
new mode 100755

I compared #27 against my own run of php composer.phar update, and the only discrepancy I found was this extraneous adding of execute permission on this one file. This patch just removes that and is otherwise identical, so leaving at RTBC. I also reviewed the non-vendor changes and they all look good to me as well, so RTBC+1.

Removing API change tag, since there's no Drupal-side API changes here.

The patch I meant to add in #32.

Committed and pushed to 8.x. Thanks!