[6.0] Added possibility to batch remove a tag

[beni71]

Pull Request for Feature request #23185 .

Summary of Changes

In the articles view there is a batch action which can add/assign one tag from the tag list to the selected articles. This PR introduces the possibility to remove/unassign one tag from the selected articles. It also extends batch processing for categories, contacts and news feeds.

How it looks like:

Testing Instructions

If you use patch tester, after applying the patch, call npm install from commandline to rebuild the .js files in folder media.

1. Make sure your Joomla tag system is active and the plugin "Behaviour - Taggable" is enabled.
2. Create a new article and in the detail view add/assign two tags, lets say "Tag1" and "Tag2".
3. Save and create a second article, add/assign it to tag "Tag2" and "Tag3".
4. Save and close.
5. In the articles list select both articles, click batch action and remove "Tag2". Apply batch processing.
6. Open the first article details and check whether the "Tag2" is removed. The tag "Tag1" must be still assigned.
7. Open the second article details and check whether the "Tag2" is removed. The tag "Tag3" must be still assigned.
8. A similar test should be done for categories, contacts and news feeds.

Actual result BEFORE applying this Pull Request

It is not possible to remove/unassign a tag from an article via batch processing.

Expected result AFTER applying this Pull Request

It is possible to remove/unassign a tag from an article via batch processing.

Link to documentations

Please select:

• [ ] Documentation link for docs.joomla.org:

• [ ] No documentation changes for docs.joomla.org needed

• [ ] Pull Request link for manual.joomla.org:

• [ ] No documentation changes for manual.joomla.org needed

There are new text keys that need to be translated.

[crommie]

I have tested this item :red_circle: unsuccessfully on d9bf8bf50f1d36213b012df640c8000df22b6585

Not yet, this throws

There is no "joomla.batch-tag-addremove" asset of a "script" type in the registry.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[beni71]

Hi @crommie, thanks for testing. I tested it again by installing patch tester on a fresh joomla installation version 5. After applying the patch the article view indeed displayed that error. After calling npm install from commandline it worked. This is needed to rebuild the .js files in folder media. I will update the testing instructions.

[crommie]

OK in that case I can't re-test since I'm testing on a PBF server site.

[ceford]

I have tested this item :white_check_mark: successfully on ba2293d923c145437409667dd5f19abea834a936

Tested batch tag removal from articles, article categories, contacts and news feeds. All OK.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[alikon]

I have tested this item :white_check_mark: successfully on ba2293d923c145437409667dd5f19abea834a936

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[alikon]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[HLeithner]

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

[crimle]

I have tested this item :red_circle: unsuccessfully on 70fa69cc67fd44d47459b24cf9081333f2aaa147

Content > Articles Error message An error has occurred. 0 There is no "joomla.batch-tag-addremove" asset of a "script" type in the registry.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[richard67]

@crimle @crommie I will reset the negative test results as they are not caused by this PR. The PR modifies an NPM dependency, which can be seen by the label "NPM Resource Changed". Such PRs can't be tested with Patchtester, they require either a development environment with composer and NPM, or they need to be applied by using the packages created by drone, which can be found in the "Downloads" section of the integration checks at the bottom of the PR.

[richard67]

Back to pending after rebase.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[richard67]

@ceford @alikon Could you test this PR again on 5.1-dev? Thanks in advance.

[ceford]

I have tested this item :white_check_mark: successfully on 70fa69cc67fd44d47459b24cf9081333f2aaa147

All tests worked fine.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[viocassel]

I have tested this item :white_check_mark: successfully on 70fa69cc67fd44d47459b24cf9081333f2aaa147

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[richard67]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40613.}

[bembelimen]

I really like this feature and it should for sure go into Joomla, sadly we can't merge it in 5.1 as it breaks B/C. So I will rebase and it should go into 6.0.

Thank you for your contribution!

[laoneo]

@beni71 can you create a manual entry for the new arguments in the files in libraries/src? These is a backwards incompatibility and needs to be documented for other extension developers.

[beni71]

@laoneo I created it within PR #242

[brianteeman]

I am confused. First @bembelimen says it cant go into 5 because its a b/c break but its ok for 6. Now @HLeithner says it cant go into 6 because its a b/c break and needs to be rewritten. And I obviously mistakenly understood that you can add options at the end just not in the middle.

[HLeithner]

Our b/c promise introduced with 5.0 said we can't b/c break "anything" without a grace period of one major version. Means introduction in 5.1 hard break (removal / behavior change) in 7.0.

Looking at this simple example on 3v4l shows you the error message for all components extending a function which gets a new parameter.

<?php

class AdminModel {
        protected function batchTag($value, $pks, $contexts, $removeTags = false) {
            
        }
}

class ModelBasedOn4or5or6code extends AdminModel {
        protected function batchTag($value, $pks, $contexts) {
            
        }
}

Error Message: Fatal error: Declaration of ModelBasedOn4or5or6code::batchTag($value, $pks, $contexts) must be compatible with AdminModel::batchTag($value, $pks, $contexts, $removeTags = false) in /in/mshfh on line 10

and it doesn't matter if it's introduced in 5.1 or 6.0, we can't break 6.0 because we say components worked with 5.x native will work with 6.0 too (at least that was the promise for 5.0 and I and most other devs expect that we keep it like this)

So changing the API for this case only works with a new function. or another way that old components still will work with 6.0. What we can do is deprecate the current signature and force extension developers to use the no signature (without functional part or proxing to a temp) function and add the new parameter in 7.0.

The other way around works: https://3v4l.org/c1ZZW

<?php

class AdminModel {
        protected function batchTag($value, $pks, $contexts) {
            
        }
}

class ModelBasedOn4or5or6code extends AdminModel {
        protected function batchTag($value, $pks, $contexts, $removeTags = false) {
            
        }
}

So to make this example more use full you can do: https://3v4l.org/iZK34

<?php

class AdminModel {
    // Change to proxy function in Joomla 7.0 to batchTag
    protected function batchTagTempTill70($value, $pks, $contexts, $removeTags = false) {
        // actuall batchTag code
    }

    // In 7.0 add $removeTags parameter and move code from Temp function to this function
    protected function batchTag($value, $pks, $contexts) {
        return $this->batchTagTempTill70($value, $pks, $contexts);
    }
}

class ModelBasedOn4or5or6code extends AdminModel {
    // Proxy function can be removed with Joomla 7.0 compatibility only 
    protected function batchTag($value, $pks, $contexts, $removeTags = false) {
        return $this->batchTagTempTill70($value, $pks, $contexts, $removeTags);
    }
}

I didn't played everything to the end but this should work.

[brianteeman]

thanks for the detailed explanation. It was just confusing as you were both saying different things so its hard to know what to do

[beni71]

@HLeithner Thanks, now I understand what you meant with b/c. Before I continue applying your suggestion, wouldn't it be easier to introduce a new function on the AdminModel like: protected function batchRemoveTag($value, $pks, $contexts)? In this case we wouldn't break compatibility, isn't it?

[HLeithner]

who calls this function? does it increase maintenance effort for us or the 3rd party dev? (2 functions need to be maintained) @Hackwar what's the plan with Tags, since we want to get rid of the implementation or better of UCM does this effect us in the future?

[Fedik]

BTW, it can be done in backward compatible way, without new method and new param. Just add a new property to the TagsHelper, and use it.

$tagsHelper->removeTags = true;
$tagsHelper->postStoreProcess();

//or
$tagsHelper->setRemoveTags(true);
$tagsHelper->postStoreProcess();

For AdminModel can use state property.

[HLeithner]

BTW, it can be done in backward compatible way, without new method and new param. Just add a new property to the TagsHelper, and use it.

$tagsHelper->removeTags = true;
$tagsHelper->postStoreProcess();

//or
$tagsHelper->setRemoveTags(true);
$tagsHelper->postStoreProcess();

For AdminModel can use state property.

of course this works but introduces a new ugly pattern which makes the code base more complex.

[beni71]

So changing the API for this case only works with a new function.

@HLeithner @Hackwar I agree. To handle b/c we could just leave batchTag function untouched but deprecate it (with a note that it will be removed in 7.0), then create a new function e.g. batchTags with the new parameter set. Wouldn't it be the easiest solution (also for devs)? Or did I forget something important?

[heelc29]

Is it possible to use func_get_args()? Like here: https://github.com/joomla/joomla-cms/blob/bdb23b84b83368a324e417c0ce8c9865cfdeca36/libraries/src/Document/HtmlDocument.php#L583-L590