[#19587] - fix undefined offset when batch copy parent menu
- Closed
- 3 Aug 2019
- Medium
- Build: staging
- # 19587
- Diff
- alikon:patch-72
- Success hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #19547.
Summary of Changes
fix undefined offset when batch copy parent menu
Testing Instructions
see #19547
all batch copy work as before
Expected result
no notice in error.log
Actual result
PHP Notice: Undefined offset: 290 in \libraries\src\MVC\Model\AdminModel.php on line 284
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
I have tested this item
PHP Version 7.1.9
Web Server Apache/2.4.27 (Win32) OpenSSL/1.0.2l PHP/7.1.9
WebServer to PHP Interface apache2handler
Joomla! Version Joomla! 3.8.5 Stable [ Amani ] 6-February-2018 15:00 GMT
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19587.
| Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
This doesn't look like a right fix. The code which set the mapping between $old and $new is for $contexts variable (which is used later in the method) is remove in this PR.
Guess we need a better fix.
Well that you've found out.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19587.
@joomdonation Not sure to understand your concern
the $contexts should be the same input for batchTag, batchLanguage, batchAccess and batchCopy methods
@alikon If you look at this line https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Model/AdminModel.php#L306 , you will see that $context parameter is used as parameter of a method call $this->$command
As your PR changes the existing value of $context variable (don't store the mapping between $new and $old in $context array anymore), I am worry that it might break something
Yeah I kind of agree, something seems off about this change (not sure what yet but just reading it it doesn't seem right).
The issue arises when copying the parent and not its children. The for loop accounts for the children even though they were not selected. So the following should fix the issue.
foreach ($result as $old => $new)
{
if (isset($contexts[$old]))
{
$contexts[$new] = $contexts[$old];
}
}
$pks:
array(1) {
[0]=>
int(238)
}
$contexts
array(1) {
[238]=>
string(18) "com_menus.item.238"
}
$result
array(3) {
[238]=>
int(506)
[445]=>
int(507)
[446]=>
int(508)
}
i'm still not convinced that we need that $old
As all here do not agree on the code, better take off the RTC?
| Status | Ready to Commit | ⇒ | Pending |
| Title |
|
||||||
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-08-03 10:10:09 |
| Closed_By | ⇒ | alikon | |
| Labels |
Removed:
J3 Issue
|
||
I have tested this item✅ successfully on 9bb1332
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19587.