New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing notice when doing batch copy and language change #5317

Merged
merged 8 commits into from Dec 8, 2014

Conversation

Projects
None yet
5 participants
@Hackwar
Member

Hackwar commented Dec 4, 2014

Fixes #5312

This PR updates the $contexts array when items are copied and at the same time, the language of the new copy is changed. This assumes that the context of each item is the same... I'm not sure that is always the case and thus someone else has to review this.

Thanks for the bug report @infograf768
Could you provide test instructions?

@infograf768

This comment has been minimized.

Member

infograf768 commented Dec 4, 2014

Test instructions:
Select a module which language is set to "All".
Click on the Batch button.
set the batch params as such:
batch change module

One will get
[04-Dec-2014 10:48:14 UTC] PHP Notice: Undefined offset: 132 in ROOT/libraries/legacy/model/admin.php on line 477
where "132" is the id of the new module

This PR does NOT solve the issue here.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 4, 2014

Could you quickly test this at your end? If not fixing it, it should at least improve it...

@infograf768

This comment has been minimized.

Member

infograf768 commented Dec 4, 2014

same

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 4, 2014

I changed this further. If this does not work, I'm really stumped... 😉

@smanzi

This comment has been minimized.

smanzi commented Dec 5, 2014

@Hackwar I tried to reproduce your issue, but I can't...

Where do you see the error message?

Here I'm using PHP 5.6.3 with error_reporting = E_ALL

@smanzi

This comment has been minimized.

smanzi commented Dec 6, 2014

@Hackwar forget it! I found it... sorry 😊

@smanzi

This comment has been minimized.

smanzi commented Dec 6, 2014

... but, with your PR applied, I'm still getting this:
[Sat Dec 06 01:13:14.349446 2014] [:error] [pid 5392:tid 1648] [client 127.0.0.1:49338] PHP Notice: Undefined offset: 91 in D:\\UniServerZ\\vhosts\\smztest\\libraries\\legacy\\model\\admin.php on line 480, referer: http://my.test.smz.it/administrator/index.php?option=com_modules&view=modules

Hackwar added some commits Dec 6, 2014

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 6, 2014

Ok, so I looked into this further and we basically have this bug everywhere where we implemented the batch feature... Please test this now again. I guess I caught all.

@smanzi

This comment has been minimized.

smanzi commented Dec 6, 2014

@Hackwar Seems to be alright now, Hannes!

While you're into the batch thing, can you look at #5304 "Problem 3"? It is a minor thing and in a way a documented "feature", but IMHO the behavior is at least... "not elegant". In #5304 (comment) I explain what I would expect as a normal behavior.

Thanks!

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 6, 2014

Hi @smanzi, I saw those issues and while we can fix those at some point in time, I would rather not complicate this PR by adding even more to it. Lets first treat this one and then tackle the rest later.

@smanzi

This comment has been minimized.

smanzi commented Dec 6, 2014

@Hackwar I totally agree! I was not asking to integrate a fix for that "glitch" into this... It was just an "Hey, take a look at that!" 😄

@smanzi

This comment has been minimized.

smanzi commented Dec 6, 2014

I don't see any regression, so... @test success!

@Kubik-Rubik

This comment has been minimized.

Member

Kubik-Rubik commented Dec 7, 2014

Tested successfully! Thank you @Hackwar for the fix.

2 tests -> RTC

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5317.

@infograf768

This comment has been minimized.

Member

infograf768 commented Dec 7, 2014

Not RTC yet. The PR works for the components patched
I see the same issue for articles, newfeeds and categories. (When patching the same way in batchCopy () as done for contact in this PR, it solves the issue here.)

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 7, 2014

Done. I plan on investing some time into the batch code... There seem to be some more improvements possible, but I will work on that in another PR.

@smanzi

This comment has been minimized.

smanzi commented Dec 7, 2014

@test success
Confirmed now working for other components too
RTC?

@Kubik-Rubik

This comment has been minimized.

Member

Kubik-Rubik commented Dec 7, 2014

Also tested the other components properly! All occurrences are now considered.

@Hackwar Thank you for the fast reaction.

Now is should be okay to set to RTC! :-)

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5317.

@infograf768

This comment has been minimized.

Member

infograf768 commented Dec 8, 2014

Merging, thanks.

@Hackwar
I guess same changes are necessary for weblinks in its own repo.

infograf768 added a commit that referenced this pull request Dec 8, 2014

Merge pull request #5317 from Hackwar/patch-18
Fixing notice when doing batch copy and language change

@infograf768 infograf768 merged commit 8b308a8 into joomla:staging Dec 8, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@infograf768 infograf768 added this to the Joomla! 3.4.0 milestone Dec 8, 2014

@Hackwar Hackwar deleted the Hackwar:patch-18 branch Dec 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment