I agree the code currently is obviously wrong.
I would love to have some tests because with this change, it will finally be possible to go into the else part of the method and I would like to have that part tested.

@okonomiyaki3000 Do you know the case where $replace isn't true and can add that as test instruction?

Unfortunately, I don't know much about this code. I just happened to spot it and thought it should be fixed.

good catch. I don't know how to test either and would ask the maintainers to merge based on review.

I don't know how to test either and would ask the maintainers to merge based on review.

The thing is, currently nothing is broken. But with this change something may be broken depending if the else part actually works or not. Until that change, that part was probably never executed.

The PR certainly is correct in that the current code is wrong. But it may still break something because another bug could surface.
Thus I asked for tests and will not merge based on simple review.

If this can help...

I did some research and the involved method is always called with the 3rd parameter either unset (hence = true by default) or explicitly set to false. This latter case happens indirectly through the setNewTags() method, defined in /libraries/joomla/table/observer/tags.php, Line 169.

The place where it is called with false are:

/administrator/components/com_categories/models/category.php (Line 744)
/libraries/legacy/model/admin.php (Line 621)

It turns out that both the aforementioned calls to setNewTags() are from identical batchTag() methods defined inside the two classes JModelAdmin and CategoriesModelCategory extends JModelAdmin.

Now:

• is it normal that in a class extending another class we have the same (almost) identical methods? Am I wrong or when extending a class it is unneeded to duplicate common code?
• almost identical because the method in admin.php declares to reurn a void while it actually returns a boolean (fixed in category.php)
• In any case it seems that the code has to do with tags batch processing

Feel free to scan through all backend models for this stuff and fix them correctly. Tags still is a mess.

@Hackwar will it be OK to remove the duplicated code in the extension class?

I did not look at the code. Maybe JModelAdmin needs fixing, too. So if it really is the same method, delete it.

@hackwar Good, thanks! (P.S.: I wrote you an e-mail: give it a look...)

@okonomiyaki3000 Elijah, if I decide to push a PR fixing all the above, it will make sense that I also include your fix. Is this OK for you?

Created #5459 which identify the problem and corrects it by extending this PR
Many thanks to @okonomiyaki3000: I didn't mean to steal your PR: your is honor of finding and fixing the issue!

Really no problem at all.

Closing this one then in favor of #5459