Feature PR-5.2-dev Pending

User tests: Successful: Unsuccessful:

avatar chmst
chmst
4 Mar 2024

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
grafik

All messages must be the same before and after patch

avatar chmst chmst - open - 4 Mar 2024
avatar chmst chmst - change - 4 Mar 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Mar 2024
Category Libraries
avatar laoneo
laoneo - comment - 4 Mar 2024

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.

avatar chmst
chmst - comment - 4 Mar 2024

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

avatar chmst chmst - close - 4 Mar 2024
avatar chmst chmst - change - 4 Mar 2024
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2024-03-04 08:30:41
Closed_By chmst
Labels Added: Updates Requested PR-4.4-dev
avatar laoneo
laoneo - comment - 4 Mar 2024

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

avatar chmst
chmst - comment - 4 Mar 2024

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

avatar joomdonation
joomdonation - comment - 4 Mar 2024

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)

avatar chmst chmst - change - 4 Mar 2024
Status Closed New
Closed_Date 2024-03-04 08:30:41
Closed_By chmst
avatar chmst chmst - change - 4 Mar 2024
Status New Pending
avatar chmst chmst - reopen - 4 Mar 2024
avatar chmst chmst - change - 4 Mar 2024
The description was changed
avatar chmst chmst - edited - 4 Mar 2024
avatar Fedik
Fedik - comment - 4 Mar 2024

@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;
}
avatar joomdonation
joomdonation - comment - 4 Mar 2024

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

avatar chmst
chmst - comment - 4 Mar 2024

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

avatar Fedik
Fedik - comment - 4 Mar 2024

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.

avatar chmst
chmst - comment - 4 Mar 2024

@Fedik what about that comment?

avatar Fedik
Fedik - comment - 4 Mar 2024

Good enough, thanks ?

avatar HLeithner
HLeithner - comment - 24 Apr 2024

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

avatar brianteeman
brianteeman - comment - 16 Jul 2024

@chmst will you be rebasing this?

avatar chmst chmst - change - 20 Jul 2024
Labels Added: Feature PR-5.2-dev
Removed: Updates Requested PR-4.4-dev
avatar Hackwar Hackwar - change - 22 Jul 2024
Title
[4.4] Remove unsed variable messages from enqueue messages
[5.2] Remove unsed variable messages from enqueue messages
avatar Hackwar Hackwar - edited - 22 Jul 2024
avatar Hackwar
Hackwar - comment - 24 Jul 2024

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

avatar Hackwar
Hackwar - comment - 24 Jul 2024

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

avatar HLeithner
HLeithner - comment - 2 Sep 2024

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

avatar HLeithner HLeithner - change - 2 Sep 2024
Title
[5.2] Remove unsed variable messages from enqueue messages
[5.3] Remove unsed variable messages from enqueue messages
avatar HLeithner HLeithner - edited - 2 Sep 2024

Add a Comment

Login with GitHub to post a comment