Flag throws [PHPUnit\Framework\Exception] session_id(): Cannot change session id when headers already sent when running tests

Just SEO purposes for people looking for this error, it comes from flag_views_query_substitutions(). The error that users have been reporting on our production site is "Warning: session_id(): Session ID cannot be changed after headers have already been sent in Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId() (lin 94 of ../vendor/symfony/http-foundation/Session/Storage/Proxy/AbstractProxy.php"

I noticed that after applying this patch I saw this error in one of my tests in which I execute a view.

Error: Call to a member function has() on null

/var/www/html/web/modules/contrib/flag/flag.views_execution.inc:18
/var/www/html/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:403
/var/www/html/web/core/modules/views/src/Plugin/views/query/Sql.php:1410
/var/www/html/web/core/modules/views/src/Plugin/views/query/Sql.php:1453
/var/www/html/web/core/modules/views/src/ViewExecutable.php:1321
/var/www/html/web/core/modules/views/src/ViewExecutable.php:1391

It appears the session does not exist yet in this case. I noticed that Drupal core PrivateTempStore calls the protected function startSession before trying to retrieve the session. See also related issue: https://www.drupal.org/node/2865991. I'm wondering.. should we make sure the session always exists?

Yea, actually, I ran into this with a view yesterday as well. I just returned an empty array if there was no session, but your suggestion seems like a better approach. Just updated the MR to ensure the session exists. I wonder if this is something that needs test coverage? I don't have a ton of experience writing tests, though, so not sure of the best way to test it.

I've tested the latest patch and it seems to be working fine for me. I haven't tried to use Flag with anonymous user by the way.

I noticed this issue shows up when using a one-time login link that eventually triggers cron. Potentially running cron as anonymous (i.e. drush) would cause similar issue? Here is the stack:

#0 /app/web/core/includes/bootstrap.inc(312): _drupal_error_handler_real(2, 'session_id(): C...', '/app/vendor/sym...', 94)
#1 [internal function]: _drupal_error_handler(2, 'session_id(): C...', '/app/vendor/sym...', 94, Array)
#2 /app/vendor/symfony/http-foundation/Session/Storage/Proxy/AbstractProxy.php(94): session_id('x-jfiy21_-Gcd4q...')
#3 /app/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php(185): Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId('x-jfiy21_-Gcd4q...')
#4 /app/web/core/lib/Drupal/Core/Session/SessionManager.php(141): Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->setId('x-jfiy21_-Gcd4q...')
#5 /app/web/modules/contrib/flag/src/FlagService.php(121): Drupal\Core\Session\SessionManager->getId()
#6 /app/web/modules/contrib/flag/src/Entity/Flag.php(201): Drupal\flag\FlagService->populateFlaggerDefaults(Object(Drupal\Core\Session\AccountProxy), NULL)
#7 /app/web/modules/contrib/flag/src/ActionLink/ActionLinkTypeBase.php(147): Drupal\flag\Entity\Flag->isFlagged(Object(Drupal\node\Entity\Node))
#8 /app/web/modules/contrib/flag/src/ActionLink/ActionLinkTypeBase.php(98): Drupal\flag\ActionLink\ActionLinkTypeBase->getAction(Object(Drupal\flag\Entity\Flag), Object(Drupal\node\Entity\Node))
#9 /app/web/modules/contrib/flag/src/Plugin/ActionLink/AJAXactionLink.php(80): Drupal\flag\ActionLink\ActionLinkTypeBase->getAsFlagLink(Object(Drupal\flag\Entity\Flag), Object(Drupal\node\Entity\Node))
#10 /app/web/modules/contrib/flag/src/FlagLinkBuilder.php(55): Drupal\flag\Plugin\ActionLink\AJAXactionLink->getAsFlagLink(Object(Drupal\flag\Entity\Flag), Object(Drupal\node\Entity\Node))
#11 [internal function]: Drupal\flag\FlagLinkBuilder->build('node', '9253', 'featured')
#12 /app/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(101): call_user_func_array(Array, Array)
#13 /app/web/core/lib/Drupal/Core/Render/Renderer.php(786): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #lazy_bu...', 'exception', 'Drupal\\Core\\Ren...')
#14 /app/web/core/lib/Drupal/Core/Render/Renderer.php(356): Drupal\Core\Render\Renderer->doCallback('#lazy_builder', Array, Array)
#15 /app/web/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, true)
#16 /app/web/core/lib/Drupal/Core/Render/Renderer.php(157): Drupal\Core\Render\Renderer->render(Array, true)
#17 /app/web/core/lib/Drupal/Core/Render/Renderer.php(578): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
#18 /app/web/core/lib/Drupal/Core/Render/Renderer.php(158): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#19 /app/web/core/lib/Drupal/Core/Render/Renderer.php(172): Drupal\Core\Render\Renderer->renderPlain(Array)
#20 /app/web/core/lib/Drupal/Core/Render/Renderer.php(663): Drupal\Core\Render\Renderer->renderPlaceholder('<drupal-render-...', Array)
#21 /app/web/core/lib/Drupal/Core/Render/Renderer.php(548): Drupal\Core\Render\Renderer->replacePlaceholders(Array)
#22 /app/web/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, true)
#23 /app/web/core/lib/Drupal/Core/Render/Renderer.php(157): Drupal\Core\Render\Renderer->render(Array, true)
#24 /app/web/core/lib/Drupal/Core/Render/Renderer.php(578): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
#25 /app/web/core/lib/Drupal/Core/Render/Renderer.php(158): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#26 /app/web/modules/contrib/search_api/src/Plugin/search_api/processor/RenderedItem.php(275): Drupal\Core\Render\Renderer->renderPlain(Array)
#27 /app/web/modules/contrib/search_api/src/Item/Item.php(272): Drupal\search_api\Plugin\search_api\processor\RenderedItem->addFieldValues(Object(Drupal\search_api\Item\Item))
#28 /app/web/modules/contrib/search_api/src/Entity/Index.php(976): Drupal\search_api\Item\Item->getFields()
#29 /app/web/modules/contrib/search_api/src/Entity/Index.php(930): Drupal\search_api\Entity\Index->indexSpecificItems(Array)
#30 /app/web/modules/contrib/search_api/search_api.module(116): Drupal\search_api\Entity\Index->indexItems(10)
#31 [internal function]: search_api_cron(Object(Drupal\ultimate_cron\Entity\CronJob))
#32 /app/web/modules/contrib/ultimate_cron/src/Entity/CronJob.php(325): call_user_func('search_api_cron', Object(Drupal\ultimate_cron\Entity\CronJob))
#33 /app/web/modules/contrib/ultimate_cron/src/Entity/CronJob.php(471): Drupal\ultimate_cron\Entity\CronJob->invokeCallback()
#34 /app/web/modules/contrib/ultimate_cron/src/Plugin/ultimate_cron/Launcher/SerialLauncher.php(213): Drupal\ultimate_cron\Entity\CronJob->run(Object(Drupal\Core\StringTranslation\TranslatableMarkup))
#35 /app/web/modules/contrib/ultimate_cron/src/Plugin/ultimate_cron/Launcher/SerialLauncher.php(334): Drupal\ultimate_cron\Plugin\ultimate_cron\Launcher\SerialLauncher->launch(Object(Drupal\ultimate_cron\Entity\CronJob))
#36 /app/web/modules/contrib/ultimate_cron/src/Plugin/ultimate_cron/Launcher/SerialLauncher.php(309): Drupal\ultimate_cron\Plugin\ultimate_cron\Launcher\SerialLauncher->runThread('1052583', 1, Array)
#37 /app/web/modules/contrib/ultimate_cron/src/UltimateCron.php(64): Drupal\ultimate_cron\Plugin\ultimate_cron\Launcher\SerialLauncher->launchJobs(Array)
#38 /app/web/modules/contrib/ultimate_cron/src/ProxyClass/UltimateCron.php(70): Drupal\ultimate_cron\UltimateCron->run()
#39 /app/web/core/modules/automated_cron/src/EventSubscriber/AutomatedCron.php(65): Drupal\ultimate_cron\ProxyClass\UltimateCron->run()
#40 [internal function]: Drupal\automated_cron\EventSubscriber\AutomatedCron->onTerminate(Object(Symfony\Component\HttpKernel\Event\TerminateEvent), 'kernel.terminat...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#41 /app/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\TerminateEvent), 'kernel.terminat...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#42 /app/vendor/symfony/http-kernel/HttpKernel.php(100): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object(Symfony\Component\HttpKernel\Event\TerminateEvent), 'kernel.terminat...')
#43 /app/vendor/stack/builder/src/Stack/StackedHttpKernel.php(32): Symfony\Component\HttpKernel\HttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\TrustedRedirectResponse))
#44 /app/web/core/lib/Drupal/Core/DrupalKernel.php(687): Stack\StackedHttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\TrustedRedirectResponse))
#45 /app/web/index.php(22): Drupal\Core\DrupalKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\TrustedRedirectResponse))
#46 {main}

@franz it does look like FlagService::populateFlaggerDefaults() calls $this->sessionManager->getId(), which I do think is the same issue solved in the MR elsewhere. Your trace includes that, so it must get hit during cron one way or the other. It looks like it could be slightly more complicated, though, as there are a couple of other methods (commented in populateFlaggerDefaults) that are supposed to "ensure that the session exists." I'd wager the handling from this MR would need to be broken apart through those methods.

Please ignore. This is a manual copy of the latest MR (https://git.drupalcode.org/project/flag/-/merge_requests/8/diffs.diff?di...); We use cweagans/composer-patches but don't want to link to gitlab MR patch urls as their content can change.

The flag module should be refactored so it no longer uses \Drupal::service('session')->getId().

Change record: https://www.drupal.org/node/3006306

The failing test is because the patch no longer uses the actual session id. Not sure how to fix the test.

You have the session ID, so you could read the value from the sessions table?

Any chance that you can find to look into updating the test soon? I'd like to roll a new 9.3 compatible release asap. I'll try to find some time myself if not.

will have a quick look at this.

Well, it's not pretty because PHP session data is annoying, but I think the partial regex there should be fairly safe, and if the session data serialization ever changes then we'll deal with it.

One thing I'm a bit concerned about is that this will initialize a session all the time, even if you don't use anonymous flags. Will have a look at that, as a starting point, we could check permissions and if anonymous flagging isn't allowed, then we don't return a placeholder value.

Closer look at the patch, it's probably OK, while we do generate such a session ID every time that function is called, only the ensureSession() method really does persist it into the session.

made some documentation improvements, also ensured that the function returns the same value if called multiple times, otherwise the property that it defines seems useless.

Committed.

This will break existing anonymous flag listings on deploy, dealing with that would be quite hard.