Use a regular flash object if session store is disabled

[ricardotk002]

Summary

After #42167, all apps (except api only ones) have access to the flash
module. If the session store is disabled, then an empty flash object
is used.

The old null flash was removed because of the maintenance costs.

cc/ @byroot

[pixeltrix]

@byroot we had the discussion about whether writing into a null session should raise or not which is a 50/50 call, but I think we should at minimum log it at the debug level to give developers a chance of seeing why their flash messages aren't appearing.

[byroot]

we had the discussion about whether writing into a null session should raise or not

Yes, and everyone agreed about writing to a disabled session being a failing call.

But in my opinion Flash is not the session, it just happens to be backed by it, and semantically it degrade fairly well.

I think we should at minimum log it at the debug level

Yes that's a good idea.

[byroot]

I don't think that it's correct to have all these methods return nil like this. Sure it will now longer break if for instance you call flash.keys, but returning nil is totally unexpected and will likely break the caller.

NullObjects are notoriously hard to get right. I'm starting to wonder if returning a regular Flash object wouldn't be better. You'd just get a new one on every request.flash call. I doubt anyone has request.flash as a hotspot so I don't think a few extra allocation are big concern.

This will also be a good place to put the log suggested by @pixeltrix

[ricardotk002]

[...] returning nil is totally unexpected and will likely break the caller.

Agree, I'll try to replicate the responses too. I also think that logs would be helpful.

I'm starting to wonder if returning a regular Flash object wouldn't be better.

Well yes, the main issue in #42167 was that the Flash middleware wasn't present when some other modules (eg. ETagWithFlash) depended on it. The null object was added for the sake of correctness I would say, as even though it is present, it basically no-ops. A regular flash object works too, with the difference that it indeed would behave as if the flash module were working just fine.

Let me know if this null object still makes sense, so that I work on both items 😄.

[byroot]

Let me know if this null object still makes sense

Well it makes sense, but maintenance wise I think it's much more work than just returning an empty Flash. So I'd go with the later.

[ricardotk002]

Sounds good. I've removed the null object, now the request should have access to a regular (empty) flash object, even when the session store is disabled.

Btw, I was checking your branch the other day and tried to add the logging you mentioned: ricardotk002@587d599c4c

Something I noticed was that the Rails session always wraps the previous session into a regular enabled one, even if that previous one comes disabled (for instance, this is what I did for the tests). I'm not sure that'd be a common case but perhaps something to keep in mind:

ricardotk002@587d599c4c#diff-40af0c0e76bdb69f4240deed5d844e4564115d291f8f73b0c4ebf523a1ed16bdR19-R23

[byroot]

we had the discussion about whether writing into a null session should raise or not

Ref: #42231

[byroot]

Could you rebase? I just merged #42231, so now you can easily check for session.enabled?

[ricardotk002]

@byroot Sorry, I'm confused. Should I re-add the null-object with full API? I removed it as per your comment #42230 (comment) and calls to session.enabled? are no longer required.

[byroot]

No I wasn't asking to re-add the null object, just to check wether this PR works well with the session change, and eventually to make sure we don't try to persist the Flash object in the session if it is disabled.

[ricardotk002]

@byroot I applied the changes you suggested. Everything seems to be working correctly. Thanks for reviewing.