I have tested this item ? unsuccessfully on d9bf8bf

Not yet, this throws

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

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.

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

I have tested this item ✅ successfully on ba2293d

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

I have tested this item ✅ successfully on ba2293d

RTC

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

I have tested this item ? unsuccessfully on 70fa69c

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

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

Back to pending after rebase.

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

I have tested this item ✅ successfully on 70fa69c

All tests worked fine.

I have tested this item ✅ successfully on 70fa69c

RTC

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!

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

@laoneo I created it within PR #242

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.

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.

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

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

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?

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.

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.

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?

Is it possible to use func_get_args()?
Like here:

Hi @beni71 we would really like to get this into Joomla 6.0 and would like to help you if we can. We agree it should be a new method and a deprecation to remove in 7.0 asap (if the message is not in before 6 then we cannot remove in 7)
Thanks for all you have done with this so far

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?

@softforge Thanks for helping me.
I have solved the last comment from @HLeithner by adding a new function postStore in the TagsHelper and deprecated the function postStoreProcess.

(if the message is not in before 6 then we cannot remove in 7)

Does it mean that I have to create a PR for the version 5.x as well? If so, shouldn't the deprecation messages be adapted to something like: deprecated 5.x will be removed in 7.0 ?

@beni71 yes you would need to get something in 5.* for deprecation in 7. As soon as we see a pr against 5.2, we can merge this one.

Thanks for the time and work on this. We can help test that and discuss with the RM for that release.

@softforge @Bodge-IT Meanwhile @beni71 has made a PR for 5.2-dev: #44383 . But as far as I understand your comments above, you just wanted to have the deprecation of the old method batchTag in 5.2-dev, not the complete change. Or did I get something wrong? The new PR #44383 seems to contain all changes from this PR here but not do anything with deprecation message. Could you check and advise?

@softforge @Bodge-IT Meanwhile @beni71 has made a PR for 5.2-dev: #44383 . But as far as I understand your comments above, you just wanted to have the deprecation of the old method batchTag in 5.2-dev, not the complete change. Or did I get something wrong? The new PR #44383 seems to contain all changes from this PR here but not do anything with deprecation message. Could you check and advise?

You are right and I have commented on #44383
Thanks for spotting it

Appreciate all your work and patience on this @beni71. Finally merged.

please create a migration entry in the manual for 6.0