[4.4] Remove unsed variable messages from enqueue messages

[chmst]

Pull Request for Issue # .

Summary of Changes

As title says, there is an unused variable.

Testing Instructions

Test different situations where messages are sent and also enqueued, like here

All messages must be the same before and after patch

[laoneo]

This should be properly tested as the function getMessageQueue initializes $this->messageQueue which is used later down. I don't think so we can remove that line. Perhaps the returned $messages variable should be used instead of $this->messageQueue.

[chmst]

Then we let it as is. Just looks weird in the IDE.

[laoneo]

What you can remove is $messages = as the $messages variable is not used anywhere.

[chmst]

shouldn't it be
$this->messageQueue = $this->getMessageQueue()

[joomdonation]

Better remove the line as you did here, and replace next line with

if (!\in_array($message, $this->getMessageQueue())) {
	// Enqueue the message.
	$this->messageQueue[] = $message;
}

$this->getMessageQueue() needs to be called here instead of just $this->messageQueue to avoid the messages queued before redirecting to new page (for example, after saving an article, we will be redirected to articles management page, the messages queued before during save process are still being displayed)

[Fedik]

@joomdonation I think with this changes we losing the context,
why it $this->getMessageQueue() and not just $this->messageQueue.

I would suggest to keep that comment and the logic. Kind of:

// For empty queue, if messages exists in the session, enqueue them first.
$this->getMessageQueue();

if (!\in_array($message, $this->messageQueue)) {
   // Enqueue the message.
  $this->messageQueue[] = $message;
}

[joomdonation]

@chmst @Fedik That also works, but it is also a bit confusing when calling method returns data but we do not use that returned data for any thing. Honestly, I don't know what's better, but I would also not complain if your code is used. The most important thing is get rid of unused variable so that the code is not being removed by mistake in the future.

[chmst]

Agree with @joomdonation
For me the comment itself is confusing and clearer to have only the code.

[Fedik]

For me the comment itself is confusing and clearer to have only the code.

The problem, that in future, someone may refactor that code, and just replace
if (!\in_array($message, $this->getMessageQueue())) {
back to
if (!\in_array($message, $this->messageQueue)) {

Because, why not?

But it is important to call $this->getMessageQueue() to load previous messages from session.
The comment is important, even if it confusing.

It can stay as you made, but then please add a comment with explanation.

[chmst]

@Fedik what about that comment?

[Fedik]

Good enough, thanks 😉

[HLeithner]

I wouldn't do this in 4.4 and move it 5.2 since it doesn't solve a bug.