Many thanks to @okonomiyaki3000: I didn't mean to steal his PR: the honor of finding and fixing the issue is totally to him.

... and if he instead wants to take the mods I made and incorporate them into his PR, I'll have no objections at all!

No no, please. Use this PR.

@okonomiyaki3000 Thanks!

Although tags are set here OK in staging, using this PR also works

@infograf768, you tried to batch-set tags on categories and it worked?

Batch: Add Tags.
Yes

That's curious... it didn't work when this PR was created. I'll re-try later. Have you seen the diff in libraries/cms/helper/tags.php ?

Yep, saw it.

You're right, it works now! Absurd... I'll re-check the conditions affected by the botched if()...

@infograf768 I reverted my test environment to the point in time when I opened this PR and described the issue and... no: even in those conditions batch setting tags on categories does work

I really don't know what had I smoked at the time: something really bad for sure.

I'm also unable to understand when and for which reasons that piece of code inside the botched if() should be executed and what can be the effects of executing it.

Essentially I'm giving up with this and of course you can freely decide what to do with this PR, if either leave things as they are (if it ain't broken, don't fix it) or accept the PR on the basis that the code is clearly formally broken (but with unknown consequences)...

Sorry for the huge f++k-up...

I would like to add that while batch setting categories tags I had those variables values:

$newTags = array(1) { [0]=> string(1) "2" }
!$newTags = bool(false)
$replace = bool(false)
$this->tagsChanged = bool(false) 

so the the code should not be executed and is not executed (!$newTags = bool(false))
I'd also add that this is the first time in my life I see a boolean operator applied to an array...

Until we clear that issue, I guess that removing the duplicate method is anyway necessary. What you think?

Yes we should fix both. The if() statement is just clearly wrong anyhow even if we can't quite replicate it yet!

Unsure: it can also happen that by "fixing" that if() something gets broken...
IMHO a deeper "code review" is needed here

#2157 This is why it was added in and it was directly related to the batch processing.

@wilsonge Thanks for the head-up! Will look into that...

I didn't apply the patch and the batch to add a new tag to categories is working.

i didnt apply the patch too, add tags ist working without the patch.

You guys are missing the point. This patch extends #5449. That patch fixes a typo which exposes a bug in the batch operation. This patch fixes that bug.

As this PR is lingering since about 5 months I decided to give it a new look and see if it was still actual.

After a thorough code review I can say it is still valid but it needs to be re-purposed: there was no "issue" to fix, but the code introduced by @okonomiyaki3000 and myself was anyway good.

Let's separately analyse the modifications introduced by this PR:

1. correct the test inside an if () ... statement (@okonomiyaki3000)
2. eliminate a duplicate method from an extended class (@smz)
3. correct a docbloc (@smz)

1.) the purpose of the code inside the botched if in JHelperTags::postStoreProcess() is to "Delete all tags data" if no $newTags = array() are passed and the $replace = true parameter is TRUE.

The affected method is called either directly or through the JTableObserverTags::setNewTags() method.

As the botched test is affected only in case of $replace being FALSE (and its default value is TRUE) we should concentrate on cases where it is either explicitly set to FALSE or can assume that value.

JHelperTags::postStoreProcess() by itself is always called without expressing the $replace parameter which then assumes the TRUE value by default and hence the botched test is not affected (good!).

setNewTags(), on the contrary is always called with its $replaceTags parameter set to FALSE and then passses it down to JHelperTags::postStoreProcess() as $replace. These, thus, can be affected cases.

But setNewTags() is called only inside the two batchTag($value, $pks, $contexts) methods - one in JModelAdmin and the second in CategoriesModelCategory (which is an extended class of JModelAdmin) - and I'm pretty sure that in those cases (we are batch processing tags...) $newTags is populated, and thus the the botched test is not affected here as well.

At the end of the story, the botched test doesn't practically affects any operation: it could only in case JHelperTags::postStoreProcess() would be called with the intention of deleting all tags data, an empty $newTags parameter and the $replace parameter set to false. In that case the method should just return; but, due to the botched test it will actually delete all tags. Happliy enough this condition doesn't seems to happen in our code right now. In any case my personal opinion is that it is correct to fix the botched test.

2.) Easy: we have the very same method verbatim duplicated in an extended class: for simplicity and code "cleanness", it should be eliminated from the extended class.

3.) Easy: a method returning boolean was documented as returning void

Now, as my original description was absolutely wrong (and test instructions were wrong as well...), I'm going to fix PR title and description.

I hope this help...

Yay