Skip to content

Unified DI and changed session initialization

Nick Sellen requested to merge simplify-di into master

What does this MR do?

On the way to implementing !543 (merged) I found the work was actually split into two parts:

  1. make it so that we only set session cookie when logging in
  2. make the session cookie persistent

To solve 1 I had to do a bunch of work to ensure we only have one DI container available. To solve 2 there is still outstanding UI work to do to work out how to get consent.

However, the work from 1 stands alone and I'd like to get that merged first as it should be ready now. It has quite a lot of changes across the codebase so could easily get out of step, and the UI part for !543 (merged) still needs more thought for a while.

Previously, due to how fSession works (it initializes the session if you try and access anything), we didn't necessarily run our own session initialization logic ($session->init()) which could mean it wouldn't have the correct settings. I added a lot more checks now so we don't let fSession initialize the session other than via our init() method.

This MR removes the Foodsharing\DI class (and so the DI::$shared container) in favour of initializing a global $container variable (using our now unified FoodsharingKernel). Ideally I would also remove this use of a global by injected more things. There are about 7 uses of it. But I would wait for the next iteration of change.

I also changed the Mem class to use instance access (was static before) and inject it via DI, it now connects on first use rather than explicit call to connect().

How confident are you it won't break things if deployed?

I wouldn't be surprised if something breaks due to the session initialization changes, but everything I tried works ok. Potentially there could also be some places where the global $container has not be initialized (e.g. if an API path access something) but I did a rough check and it looks like only non-API-type things use it.

Links to related issues

Checklist

  • added a test, or explain why one is not needed/possible... (existing tests should be ok)
  • no unrelated changes
  • asked someone for a code review
  • joined #foodsharing-beta channel at https://slackin.yunity.org
  • added an entry to CHANGELOG.md (description, merge request link, username(s))
Edited by Chris Oelmueller

Merge request reports