flashNow() messages persist beyond the current request

[timdev]

Bug Report

Summary

Messages set with flashNow() persist beyond the current request. I found this surprising and inconvenient, but feel like maybe it's intended. So I'll explore it here and suggest some potential changes.

Current behavior

Messages added via flashNow() are visible immediately, in the request in which they're added, and again in the subsequent request. Note that this does not contradict the documentation, which says:

When you create a flash message, it is available in the next request, but not the current request. If you want access to it in the current request as well, use the flashNow() method instead of flash():

(Emphasis mine)

If this is in fact the expected behavior, I'm somewhat astonished.

How to reproduce

Implement the fairly common pattern of a form submission that redirects on success, but re-renders the form in the response to the POST on error. Use flashNow() in the error branch to display a message in the response (relying on the default $hops = 1) argument value). Resubmit the form so that the submission succeeds, and and the response redirects the user. Note that the error-info set via flashNow() is now displayed a second time.

Expected behavior

I'd expect messages set with flashNow() to be visible exclusively in the current request, at least by default.

A strongly-opinionated change would be to simply remove the call to $this->flash(...) in the flashNow() implementation (and remove the $hops argument from flashNow() as well, I suppose).

A more flexible fix would be to allow flash() to accept a $hops of zero, and change the default for flashNow() to zero $hops.

Unfortunately, both of those options break BC. A BC-preserving fix would be to simply allow zero $hops. Then users of the class could at least get the behavior they'd like out of the implementation.

As it stands now, I had to "write" my own implementation of FlashMessagesInterface that behaves as I'd expect. But that implementation was "copy FlashMessages and remove one line", which feels dirty and makes me sad.

Finally, I'll say that I feel like I'm probably missing some obvious use-case where the current behavior is expected. If someone enlightened me, my consternation might evaporate.

[weierophinney]

I can't recall the exact rationale why we chose for this to work the way it does; it may have been a misunderstanding of how Rails' flash.now works.

But let me turn this around on you: if you need a message that's only available for the current request... why use the flash functionality at all? Why not just pass a variable to the view script and test for that? What is the point of using the flash system for the current request in the first place? Answer these might help us better understand if this is a documentation problem, or something we should resolve (likely with a BC break).

[timdev]

Regarding my use case, it's mostly laziness. I've got flash-stuff implemented, with display code in the layout template already. I've got a couple of forms with enough client-side validation that POSTs should generally succeed. But something can always go wrong. Client side-validation might be broken resulting in the server-side validating not passing, an anti-CSRF token might be expired, or the form-handling code could encounter some runtime exception, and we'd want some vague message that exhorts the user to "try again. If this issue persists contact us.".

Since I've already got a mechanism in the presentation code to appropriately display such things, it's tempting to use it, and avoid time spent building and maintaining a bunch of view layer code just to handle cases that should only very rarely occur.

I'm sympathetic to the idea that flashNow() shouldn't exist at all. But since it does, it seems like it ought to support only-visibile-in-this-request usage.

If we wanted to avoid a BC we could simply change the minimum (not default) number of $hops to zero. I don't think that would be considered a BC break, and would at least allow users with use-cases like mine to use the implementation provided.

[froschdesign]

@timdev

Note that this does not contradict the documentation, which says:

The same is documented in the comments of the interface:

mezzio-flash/src/FlashMessagesInterface.php

Lines 44 to 53 in 460b5ad

	/**
	* Set a flash value with the given key, but allow access during this request.
	*
	* Flash values are generally accessible only on subsequent requests;
	* using this method, you may make the value available during the current
	* request as well.
	*
	* @param mixed $value
	*/
	public function flashNow(string $key, $value, int $hops = 1): void;

If we wanted to avoid a BC we could simply change the minimum (not default) number of $hops to zero.

Caution: the related exception is based on the value 0:

mezzio-flash/src/Exception/InvalidHopsValueException.php

Lines 17 to 27 in 460b5ad

	class InvalidHopsValueException extends InvalidArgumentException implements ExceptionInterface
	{
	public static function valueTooLow(string $key, int $hops): self
	{
	return new self(sprintf(
	'Hops value specified for flash message "%s" was too low; must be greater than 0, received %d',
	$key,
	$hops
	));
	}
	}

[froschdesign]

@weierophinney

if you need a message that's only available for the current request... why use the flash functionality at all? Why not just pass a variable to the view script and test for that?

If you want the same layout for the messages, with colours, icons, etc., why duplicate all these things?

[timdev]

It's not clear to me whether allowing zero hops (without changing any default argument values) would be considered a BC break. I tend to think not, though I suppose someone could theoretically depend on that behavior.

If such a change is BC, I'd be happy to submit a PR.

If it would be considered a BC break, we might entertain the idea of actually changing the default to be less surprising to people who've previously used rails, slim-flash, etc.

[froschdesign]

I tend to think not, though I suppose someone could theoretically depend on that behavior.

I think the following call might be helpful:

$flashMessages->flashNow($messageName, $messageValue, 0);

But the handling of the hops and the exception must be changed for this and therefore I think it is a problem.

less surprising to people who've previously used rails, slim-flash, etc.

Less surprising? slim-flash does not document the feature at all, that's what I call surprising. 😉

There is no clear standard for flash messages itself and there are many different implementations. For example:

• In Symfony there is a peek method to get a message in the current request but this method does not remove the message from internal storage.
• In TYPO3, a message is not saved in a session by default, but if the related help function is called in a controller, then it is saved in a session.
• Some other systems only support the basic function: the transport from the current request to the next request. Nothing more.
• …

[timdev]

I think the following call might be helpful:

Yes, I'd be satisfied if that call didn't throw. Would the switch from "must be greater than zero" to "must not be negative" be a BC violation?

slim-flash does not document the feature at all, that's what I call surprising

Slim used to document it, FWIW. Funny enough, correcting the phpdoc for that method is the most recent commit in the repo

There is no clear standard for flash messages itself

That's true.

I still find the current behavior astonishing. Maybe it's lack of imagination, but I haven't been able to come up with a use case where the current behavior is what you'd want.

[froschdesign]

Yes, I'd be satisfied if that call didn't throw.

The behaviour if the value is too low is explicitly tested:

mezzio-flash/test/FlashMessagesTest.php

Lines 346 to 370 in 460b5ad

	public function testCreationAggregatesThrowsExceptionIfInvalidNumberOfHops()
	{
	$this->expectException(InvalidHopsValueException::class);
	$this->session
	->expects($this->once())
	->method('has')
	->with(FlashMessagesInterface::FLASH_NEXT)
	->willReturn(false);
	$this->session
	->expects($this->never())
	->method('get')
	->with(FlashMessagesInterface::FLASH_NEXT, [])
	->willReturn([]);
	$this->session
	->expects($this->never())
	->method('set')
	->with(
	$this->anything(),
	$this->anything()
	);
	$flash = FlashMessages::createFromSession($this->session);
	$flash->flashNow('test', 'value', 0);
	}

Slim used to document it, FWIW.

Version 2 is not the freshest version. I checked the last two versions and there was nothing on this.

I still find the current behavior astonishing.

Make a suggestion via a pull request!

[timdev]

Make a suggestion via a pull request!

I'm willing to do that, but I'll be sad if it's considered a BC violation and has to wait for a 2.0 release which, frankly, may never arrive given the relative simplicity of this component. If "merely" adjusting the validation breaks BC, then I'd propose additional changes - though it might be as simple as changing the default for $hops in flashNow's argument list.

[froschdesign]

I'm willing to do that, but I'll be sad if it's considered a BC violation and has to wait for a 2.0 release which, frankly, may never arrive given the relative simplicity of this component.

The other way round: it is easier to create a new major version with this "simple" component.

[timdev]

Okay, since I'm somewhat fussy, there are now two PRs (#8 and #9) for your consideration. The former (IMO) preserves BC while enabling my use-case, while the latter is a BC break based on my opinion of what's least astonishing.

[boesing]

Hm, without diving too deep in here:
I think, keeping BC by adding the ability to pass 0 to the flashNow method is absolutely okay. Both approaches on getting this feature are using 0 for the hops argument.

Imho, we can go with #8 and probably re-target #9 to the next major of this package?

/cc @weierophinney