Move ini_set calls before a new session is started

[DeepDiver1975]

Description

Alternative approach to #31525

Inspired by https://github.com/joomla/joomla-cms/pull/15742/files

Related Issue

• Fixes PHP 7.2.5 ini_set(): A session is active #31524

How Has This Been Tested?

• manually

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

Open tasks:

• Backport (if applicable set "backport-request" label and remove when the backport was done)

[codecov]

Codecov Report

Merging #32298 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master   #32298      +/-   ##
============================================
- Coverage     64.01%   63.99%   -0.02%     
- Complexity    18560    18564       +4     
============================================
  Files          1171     1171              
  Lines         69837    69857      +20     
  Branches       1267     1267              
============================================
  Hits          44706    44706              
- Misses        24761    24781      +20     
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	52.84% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	65.27% <0%> (-0.03%)	18564 <5> (+4)

Impacted Files	Coverage Δ	Complexity Δ
lib/base.php	5.97% <ø> (+0.05%)	125 <0> (-1)	⬇️
lib/private/Session/Internal.php	0% <0%> (ø)	24 <5> (+5)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23c9335...194b9c5. Read the comment docs.

[patrickjahns]

@DeepDiver1975

I don't quite understand why you move non-session related ini_set methods also into the session part

Wondering about the Request object "helper" -> we could follow the symfony approach where we create a request object early on before the "Kernel" is bootstrapped

https://symfony.com/doc/current/components/http_foundation.html#request

Example: https://github.com/Sylius/Sylius/blob/master/web/app.php#L23-L27

[DeepDiver1975]

I don't quite understand why you move non-session related ini_set methods also into the session part

I was a bit overmotivated as it seems ... 🤕

Wondering about the Request object "helper" -> we could follow the symfony approach where we create a request object early on before the "Kernel" is bootstrapped

Our request object is already too big and has too much dependencies - initializing the request object at the wrong point in time leads to severe issues .....

[DeepDiver1975]

@patrickjahns please review once more - thx

[patrickjahns]

Our request object is already too big and has too much dependencies - initializing the request object at the wrong point in time leads to severe issues .

Hence the idea to have a slim Request::createFromGlobals(); -> all other things on the request should be set over time

If we think about a Middleware Handler approach, with each Middleware the request object could be either extended or wrapped to be passend down the line with more information.

So we get rid of the request object dependencies and have a clear request -> response cycle

[patrickjahns]

I know this is just a code move - but do we have more information why this is? Wondering if this still holds true today

[DeepDiver1975]

This comment is basically just copied over from the php docs - http://php.net/manual/en/session.security.ini.php

[DeepDiver1975]

Hence the idea to have a slim Request::createFromGlobals(); -> all other things on the request should be set over time

If we think about a Middleware Handler approach, with each Middleware the request object could be either extended or wrapped to be passend down the line with more information.

So we get rid of the request object dependencies and have a clear request -> response cycle

fully understood the concept - no need to explain - question is how to do the transition. We basically need to remove the csrf check from IRequest - then the dependency cylce issue is gone.

Not that critical ... we need to double check -> different PR

[patrickjahns]

Not that critical ... we need to double check -> different PR

👍

So for this PR to merge - I wonder if can get ui tests to run with https. That way the cookie change to move to https would be tested in the sessions.

Needs some investigation @phil-davis @individual-it

[individual-it]

@patrickjahns there are ways to auto accept self signed SSL certificates in selenium, would need a practical try to see if they work for us

[individual-it]

opened issue for running tests with SSL: owncloud/QA#581

[patrickjahns]

@DeepDiver1975
For approving I really would like to see https running - otherwise the change in behavior wouldn't be tested

[miqrogroove]

Works for me.

[PVince81]

@patrickjahns merge for master but block backport until acceptance tests are there ?

having it in master will at least have this run on developers envs which might confirm further that it works

[patrickjahns]

Merging for master 👍

[patrickjahns]

@PVince81 - lets go ahead

[PVince81]

stable10: #32538