I have tested this item ✅ successfully on c72d6f2

I've tested with 2 articles which had differenc ACL modifications.
Both articles copied in 1 batch action.
Before patch: Copied articles have ACL like in new articles.
After patch: Each of the article copies has ACL like the particular copy source article.

@HLeithner and @wilsonge Release leads please check as also with PR #24730 if this is a bug fix so it goes into 3.9.next or a new feature for 4.0.

I have tested this item ✅ successfully on c72d6f2

I've tested with 2 articles which had differenc ACL modifications.
Both articles copied in 1 batch action.
Before patch: Copied articles have ACL like in new articles.
After patch: Each of the article copies has ACL like the particular copy source article.

@HLeithner and @wilsonge Release leads please check as also with PR #24730 if this is a bug fix so it goes into 3.9.next or a new feature for 4.0.

I have tested this item ✅ successfully on c72d6f2

Status "Ready To Commit".

I'm not unhappy to call this a bug fix - but what happens if there isn't a entry in the permissions table for the asset - shouldn't we check in JTable that there actually is a asset_id for items

I agree, for this one, with @wilsonge
I guess we may not need that check for categories and modules.

Agreed. The stuff that's component specific is fine. But this generic one needs more careful sanity checks

not so sure what "sanity check" you are talking about if target and/or source asset don't exists the query simply does nothing

I guess just making sure there is an existing asset for the original item.

if ($oldAssetId)

query

@wilsonge
the rtc label has been taken off without a reply to @alikon
Please explain further what you expect to get this in.

I removed it because George requested more sanity checks, I'm also not sure if this is a bugfix because permission copying is missing or a new feature.

Atm I think it's a bugfix but ymmv.

for sanity check i've already answered #24736 (comment)
😕

@wilsonge could you please elaborate your comment?

Inside JTable we check for asset support based on the asset id field being present

we need something similar in the model so that we know if the item actually has an asset or not because right now we're just going to blindly try and clone assets even for items that don't have assets

did you mean something like $this->table->hasField($this->table->getColumnAlias('asset_id'))
?

Something like that yes

Back to pending until change.

change made, please check, retest

I'll wait for the travis and appveyor and drone results, then test. Am already preparing.

@alikon I get this after having applied your patch on a current staging, clean new install with testing sample datas, using patchtester to apply it:

yes you're right hasField() was introduced in 4.x backported to 3.x now

@alikon Now there is no error, and the articles are copied, but the permissions not.

@alikon Sorry, forget previous comment. My mistake.

@alikon Hmm, now after having applied the patch, when batch copying 2 articles, only permissions of 1st article are copied.

@alikon Sorry, again my mistake. all ok.

I have tested this item ✅ successfully on c0fbbeb

I have tested this item ✅ successfully on c0fbbeb

I have tested this item ✅ successfully on 7b6861a

I have tested this item ✅ successfully on 7b6861a

In PHP error log when batching banners.

PHP Notice: Undefined property: BannersTableBanner::$asset_id in \joomla-cms-staging\libraries\src\MVC\Model\AdminModel.php on line 439

In PHP error log when batching banners.

PHP Notice: Undefined property: BannersTableBanner::$asset_id in \joomla-cms-staging\libraries\src\MVC\Model\AdminModel.php on line 439

Confirmed.

I have not tested this item.

I have not tested this item.

I have tested this item ✅ successfully on 49226ba

@richard67 can you please retest?

@franz-wohlkoenig As soon as I have cooked and eaten I will re-test.

I have tested this item ✅ successfully on 49226ba

1. Permissions are copied.
2. No "PHP Notice: Undefined property ..." anymore when batch copying banners.

I have tested this item ✅ successfully on 49226ba

1. Permissions are copied.
2. No "PHP Notice: Undefined property ..." anymore when batch copying banners.

@franz-wohlkoenig Done.

Status "Ready To Commit".

@alikon can you please do the cleanup i mentioned

@wilsonge I did PR #51 as you suggested, but had to revert with PR #52 due to a warning that I can't remember at the moment.

@wilsonge to me the only thing which i can think about is to move

outside i.e before the while cause we are always referring to the same table.....
but $oldAssetId must be stored before $this->table->asset_id is changed by this->table->store()

or i missunderstand you

@wilsonge can you comment on this before I merge it?

Basically what @Quy did in https://github.com/alikon/joomla-cms/pull/51/files is what I’m after - it makes more sense if we can make it work. Can we find out why that was failing?

@alikon why closing this?

@alikon Did you maybe accidently hit the wrong button, close instead of rebase?

i've had 5 minutes of "send all to the hell" feeling...
...a little bit better now

@wilsonge The syntax for the join, does the fix for the "ON" which you have done in J4 here a18322d apply also to J3? If so, it should be also fixed here in this PR. I will then make PR for Nicola's branch but want you to confirm if that would be right.

@wilsonge I just see according to J3 database api doc my previous comment for you is wrong, i.e. this PR here is ok with "... ON ... " in 1 join condition. Is that right?

Regarding your sanity check, Nicola has merged noth PRs from @Quy so now it should be ok. Maybe reason why moving things down did not work is because the $this->table->get('id'); would be before that then and reset stuff somehow?

@HLeithner cause i'm not too much able to read only & fully understand code
i've made a test with your suggestion applyied ...... as a result permission are not copyied

so may I ask you to do the same test?
with your suggestion and without (only this silly pr)
........maybe you'll can confirm or deny my results

@Quy That was what I supposed, too, but seems that comment in code reviews was too hidden.

Tonight I can test again. But last changes were code style only so my old test should still be ok.

I have tested this item ✅ successfully on 6b24187

I have tested this item ✅ successfully on 6b24187

@viocassel or @Quy could you test again? Thanks in advance.

I have tested this item ✅ successfully on 6b24187

I have tested this item ✅ successfully on 6b24187

@viocassel Thanks for testing.

Status "Ready To Commit".

Thank you for your engagement in Joomla!