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.

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

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

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

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)

@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;
}

@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.

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

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.

@Fedik what about that comment?

Good enough, thanks 😉

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

@chmst will you be rebasing this?

Could those who worked on this PR already test it again? That way it could be merged into 5.2.

The PR has been rebased. Could you retest this so that we can merge this into 5.2?

This pull request has been automatically rebased to 5.3-dev.

I have tested this item 🔴 unsuccessfully on 208df7a

Installing the patch resulted in an error that crashed my whole Test website:
Call to undefined method Joomla\CMS\Application\AdministratorApplication::checkUserRequiresReset()

I have tested this item 🔴 unsuccessfully on 208df7a

Can confirm, same error after apply with patchtester.

I have tested this item ✅ successfully on 26125e1

Testet again after rebase, now all works as expected.

I have tested this item ✅ successfully on 26125e1

I used the workflow and set it so that it was not correctly set up, that produced both an error and a saved item message, the test instructions followed adn the results as suggested

RTC

Thanks!