Skip to content
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

Don't delete children if we iterate over all of them anyway #16428

Closed
wants to merge 1 commit into from

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented Jun 1, 2017

Pull Request for Issue #15929.

During installation of a component, all existing menuitems of that component are deleted and recreated afterwards.
This is done by fetching all menuitems of said component using a database query and then iterating over each of them and deleting it using JTableNested::delete().
That method takes a second option argument which defaults to true and makes sure that children are deleted as well.
Now that is nice but since we are iterating over all menuitems, it means that in the first iteration the parent is deleted and all successive deletes of the children fail and generate an error that _getNode can't find the item.

The difference between J3.7 and J4 is that in J3.7 the errors are pushed into $this->_errors but neither the return value (false) nor that error array is ever checked afterwards.
In J4 a warning is raised and thus those errors suddenly become visible.

Summary of Changes

This changes the call so it doesn't delete the children.

Testing Instructions

In J3 there isn't much you can test beside that menuitems are deleted/recreated as expected after updating an extension with a submenu (eg SermonSpeaker https://www.sermonspeaker.net/download/sermonspeaker-component/sermonspeaker-component-5-6-2/com_sermonspeaker-zip.zip).

In J4 this will prevent the error messages mentioned in the above issue.

Expected result

Component updates as expected

Actual result

Component updates as expected

Documentation Changes Required

None

@ghost
Copy link

ghost commented Jun 2, 2017

Install com_sermonspeaker.zip in Joomla4 got Internal Server Error.

System information

4 (latest nightly build)
macOS Sierra, 10.12.5
Firefox 53 (64-bit)

MAMP 4.1.1

  • PHP 7.0.15
  • MySQLi 5.6.35

@Bakual
Copy link
Contributor Author

Bakual commented Jun 2, 2017

Yep, that zip only works in J3 due to the various B/C breaks J4 will bring.
For J4, you will have to install my development version:
com_sermonspeaker.zip

@Bakual
Copy link
Contributor Author

Bakual commented Jun 2, 2017

Be aware that this PR is for J3 and needs to be merged up into J4 afterwards. You can't apply it to J4 in the current form.

@ghost
Copy link

ghost commented Jun 3, 2017

Installed and uninstalled Sermonspeaker in Joomla 3: All submenus in Table _menu are deleted. I'm not sure if this is an successfully test.

@Bakual
Copy link
Contributor Author

Bakual commented Jun 3, 2017

Expected behavior:
When installing a component, the menus are created.
When updating a component, the menus are deleted and recreated without any errors.
When uninstalling a component, the menus are deleted.

As said, there isn't much to test other than the behavior is the same as before, meaning the menus are there after installing/updating and gone after uninstalling.
The errors which happend upong deleting the menus don't show in J3.

@Quy
Copy link
Contributor

Quy commented Jun 22, 2017

I have tested this item ✅ successfully on 29bc7ca


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

@ghost
Copy link

ghost commented Oct 26, 2017

@Bakual so if its expected Behavior i can mark Test as successfully?


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

@Bakual
Copy link
Contributor Author

Bakual commented Oct 26, 2017

Yep, I think so.

@ghost
Copy link

ghost commented Oct 26, 2017

I have tested this item ✅ successfully on 29bc7ca


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

@ghost
Copy link

ghost commented Oct 26, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 26, 2017
@ghost
Copy link

ghost commented Oct 26, 2017

@Bakual can you please resolve conflicting Files?

@alikon
Copy link
Contributor

alikon commented Oct 26, 2017

is this the same of #18404

@Bakual
Copy link
Contributor Author

Bakual commented Oct 26, 2017

Looking at its code, I'd say yes. Classname was namespaced which is the conflict we have here.
Lets use his code.

Closing in favor of #18404

@Bakual Bakual closed this Oct 26, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 26, 2017
@Bakual Bakual deleted the DontDeleteChildren branch October 26, 2017 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants