Fix menu item deletion #15500
Fix menu item deletion #15500
Conversation
|
A search for the keyword 'Content.Remove' throughout the solution reveals many similar operations. I guess they're broken, too
|
|
@sarahelsaig @MikeAlhayek could you please check? |
|
It is best to find all references to |
| @@ -270,16 +270,18 @@ public async Task<IActionResult> Delete(string menuContentItemId, string menuIte | |||
| return NotFound(); | |||
| } | |||
|
|
|||
| var menuContentAsJson = (JsonObject)menu.Content; | |||
There was a problem hiding this comment.
ContentElement.Content maybe is a instance of JsonDynamicObject
src/OrchardCore.Modules/OrchardCore.Menu/Controllers/AdminController.cs
Outdated
Show resolved
Hide resolved
| var menuItems = menuContentAsJson[nameof(MenuItemsListPart)]["MenuItems"] as JsonArray; | ||
| menuItems.Remove(menuItem); |
There was a problem hiding this comment.
Sorry for the nitpick, but why did you use as JsonArray here? Is it possible that this won't be here or won't be the right type? Because right now it will just cause an NRE on the next line in that scenario. I suggest either doing null-forgiving on the Remove as well (menuItems?.Remove(menuItem);) or throwing an explicit exception.
There was a problem hiding this comment.
Okay, I may have another problem
There was a problem hiding this comment.
@sarahelsaig updated , at #15509
There was a problem hiding this comment.
Sorry for the nitpick, but why did you use
as JsonArrayhere? Is it possible that this won't be here or won't be the right type? Because right now it will just cause an NRE on the next line in that scenario. I suggest either doing null-forgiving on the Remove as well (menuItems?.Remove(menuItem);) or throwing an explicit exception.
Because there was a logical error in the last change here
There was a problem hiding this comment.
Sorry, my English is not good. You can debug the original code, which attempts to remove a menuId directly from a jsonObject
There was a problem hiding this comment.
Sorry for the nitpick, but why did you use as JsonArray here? Is it possible that this won't be here or won't be the right type? Because right now it will just cause an NRE on the next line in that scenario. I suggest either doing null-forgiving on the Remove as well (menuItems?.Remove(menuItem);) or throwing an explicit exception.
The MenuItems is always an array
There was a problem hiding this comment.
The MenuItems is always an array
Thanks for the clarification.
You can debug the original code, which attempts to remove a menuId directly from a jsonObject
I've tried understanding JsonDynamicObject. It's the same as the inner _jsonObject, except it also has a _dictionary. This _dictionary field seems like a cache to me. So this could be fixed by emptying it after the json object is updated.
There was a problem hiding this comment.
@hyzx86 I've fixed it using the additional "Remove" methods from your branch. Please check out my PR (hyzx86#1003).
|
@sarahelsaig could you please approve this PR if you don't have any objection |
|
@sarahelsaig @hyzx86 can we continue merging related PRs if they are ready |
Fixes #15497
/cc @sarahelsaig